-
Notifications
You must be signed in to change notification settings - Fork 262
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
Setup datadog #1462
Setup datadog #1462
Conversation
@@ -74,7 +75,7 @@ pub async fn handle( | |||
}; | |||
|
|||
// Record current service for tracing purposes | |||
span.record("service", &service); | |||
span.record("shuttle.service", &service); |
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.
service
is already used to designated the Shuttle service (logger, gateway, ..). Might want to even rename it to something like user.service
. But this is why it was renamed.
} | ||
} | ||
|
||
// TODO: Remaining field types from AnyValue : Bytes, ListAny, Boolean |
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.
Do we want to cover this TODO before upgrading from draft PR? I guess there will be compile time errors in case the types need to be implemented, so not sure if it is worth it in case we don't have errors at this point.
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.
It's from the original file, I'm not even sure it's relevant, and we don't need to address it now.
- job_name: "otelcol" | ||
scrape_interval: 10s | ||
static_configs: | ||
- targets: ["0.0.0.0:8888"] |
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 this indentation needed?
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.
It was autoformatted, so usually yes, even though the previous one might also be correct (because YAML is a weird language).
))); | ||
} | ||
|
||
// add the `name` metadata to attributes |
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 this referring to the span name? I don't see why we don't want to add it to log_record.attributes
. The result, as I see, is to be able to look for the spans with a given name and check the logs emitted for them.
Is this how we'll benefit from adding the name to the log_record.attributes
?
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.
Left over from the previous code that I removed because we don't actually need it.
} | ||
|
||
// add the `name` metadata to attributes | ||
// TBD - Propose this to be part of log_record metadata. |
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.
By metadata here you refer to attributes
?
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.
Part of the original file as well.
log_record.trace_context = Some(TraceContext::from(&SpanContext::new( | ||
trace_id, | ||
span_id, | ||
TraceFlags::default(), | ||
false, | ||
TraceState::default(), | ||
))); |
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.
Are the logs attached to the spans (visible in datadog) because of attaching this trace_context
to them, which contains the trace_id
and span_id
?
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.
Yes exactly.
- Since Datadog isn't using the `events` from the span to setup logs, we instead rely on the logging pipeline from OpenTelemetry. - Fix the Otel collector configuration to correctly work with Datadog: - Correctly set the environment key (need to use the OPTL way to set it, not Datadog). - Transform the resource.name from name to have span show the right resource. Done both through a transfo and with span_name_as_resource_name. - Change in our tracing configuration for span to use `service.name` instead of `service`, because that's the official OPTL way of doing. fix(otlp): export to honeycomb still build: update Cargo.lock
We shouldn't add everything as attributes to the span from the arguments. And we should avoid logging the display value of some values, and instead reach for the string. Examble with the FQDN, we want to have the hostname, not `FQDN(\x06foo.shuttleapp.rs\n)`.
Not necessary anymore since we're not relying on ingesting stdout logs.
This messes up Datadog understanding of this tag, for some reason.
Co-authored-by: Iulian Barbu <[email protected]>
Description of change
DON'T MERGE
The
SHUTTLE_ENV
thing is needed to be able to differentiate between, well, environments, in particular when running locally vs staging. We could useDD_ENV
for that, but I think it's better to have a dedicated env for us.Regarding the actual Datadog changes:
Log correlation + correctly named spans.

Right resource, right service in the traces.

Actual logs, correlated, with the right service.

How has this been tested? (if applicable)
Ran it locally, connected to Datadog.