-
Notifications
You must be signed in to change notification settings - Fork 171
[worker] - log unknown build error logs in Datadog #3386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Subscribed to pull request
Generated by CodeMention |
|
Size Change: -1.61 kB (0%) Total Size: 72.7 MB
|
packages/worker/src/service.ts
Outdated
| const rawMessage = | ||
| maybeRawError instanceof Error ? maybeRawError.message : 'An unknown error occurred'; | ||
| void datadogLogs.send({ | ||
| message: `Unknown build error: ${rawMessage}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is "unknown build error:" helpful here?
packages/worker/src/service.ts
Outdated
| void datadogLogs.send({ | ||
| message: `Unknown build error: ${rawMessage}`, | ||
| level: 'error', | ||
| tags: { build_id: this.buildId }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the cardinality on this ok? I've heard high cardinality is where we would pay big bucks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesnt apply to logs fields, that pricing is for custom metrics.
Adding a high-cardinality tag like a Build ID to your logs does not directly increase your per-log "Log Management" price, but it can trigger massive costs elsewhere in your Datadog bill.
Here is how that high-variable tag affects different parts of your bill:
- Log Management (Ingestion & Indexing)
Direct Cost: There is no extra charge for adding more tags or high-cardinality attributes to your logs.
The "Weight" Impact: Adding a long tag string increases the size (bytes) of each log. Since you are billed for ingestion by the gigabyte (~$0.10/GB), a very large number of long tags could slightly increase your ingestion costs over time.- The Trap: "Log-Based Metrics"
The real danger is if you use that tag in a Log-Based Metric.
The Cost: Datadog charges for custom metrics based on the number of unique tag combinations (cardinality).
The Result: If you create a metric from your logs and include the build_id tag, every single build will create a new "unique time series." This can quickly exceed your custom metric allotment and lead to thousands of dollars in overage fees ($0.05 per metric/month).
packages/worker/src/service.ts
Outdated
| if (err.errorCode === errors.ErrorCode.UNKNOWN_ERROR) { | ||
| const rawMessage = | ||
| maybeRawError instanceof Error ? maybeRawError.message : 'An unknown error occurred'; | ||
| void datadogLogs.send({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| void datadogLogs.send({ | |
| datadogLogs.send({ |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note technically we might finish the build before we get to send the request. Might be ok but worth mentioning.
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'DD-API-KEY': this.apiKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If worker has access to DataDog API key we could as well set it here — users are going to have access to it.
For step metrics we decided to relay these through www. Why can we not do it for this?
| interface DatadogLogsOptions { | ||
| apiKey: string | null; | ||
| site: string; | ||
| service: string; | ||
| logger: bunyan; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does AI like to extract these interfaces so much instead of inlining in the constructor…
| ]), | ||
| }); | ||
| } catch (error) { | ||
| this.logger.error(error, 'Failed to send log to Datadog'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.logger.error(error, 'Failed to send log to Datadog'); | |
| this.logger.error({ err: error }, 'Failed to send log to Datadog'); |
This is bunyan nomenclature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3386 +/- ##
==========================================
+ Coverage 49.63% 52.31% +2.69%
==========================================
Files 673 804 +131
Lines 28355 33425 +5070
Branches 5903 6975 +1072
==========================================
+ Hits 14070 17482 +3412
- Misses 13082 14555 +1473
- Partials 1203 1388 +185 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |
Why
Record Unknown Errors with Datadog logs so that we can index by build ID, track patterns, and determing regressions/fixes. More detailed here https://github.com/expo/universe/pull/24771
How
Use Datadogs log ingestion API https://docs.datadoghq.com/api/latest/logs/#send-logs