Skip to content
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

Merged
merged 13 commits into from Dec 4, 2023
Merged

Setup datadog #1462

merged 13 commits into from Dec 4, 2023

Conversation

Kazy
Copy link
Member

@Kazy Kazy commented Dec 3, 2023

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 use DD_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.
    image

  • Right resource, right service in the traces.
    image

  • Actual logs, correlated, with the right service.
    image

How has this been tested? (if applicable)

Ran it locally, connected to Datadog.

@Kazy Kazy requested a review from chesedo December 3, 2023 13:29
@@ -74,7 +75,7 @@ pub async fn handle(
};

// Record current service for tracing purposes
span.record("service", &service);
span.record("shuttle.service", &service);
Copy link
Member Author

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.

@Kazy Kazy marked this pull request as draft December 3, 2023 13:43
Cargo.lock Outdated Show resolved Hide resolved
}
}

// TODO: Remaining field types from AnyValue : Bytes, ListAny, Boolean
Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines +30 to +33
- job_name: "otelcol"
scrape_interval: 10s
static_configs:
- targets: ["0.0.0.0:8888"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this indentation needed?

Copy link
Member Author

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).

gateway/src/project.rs Outdated Show resolved Hide resolved
)));
}

// add the `name` metadata to attributes
Copy link
Contributor

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?

Copy link
Member Author

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.
Copy link
Contributor

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?

Copy link
Member Author

@Kazy Kazy Dec 4, 2023

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.

Comment on lines +118 to +124
log_record.trace_context = Some(TraceContext::from(&SpanContext::new(
trace_id,
span_id,
TraceFlags::default(),
false,
TraceState::default(),
)));
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly.

@Kazy Kazy marked this pull request as ready for review December 4, 2023 12:41
Kazy and others added 12 commits December 4, 2023 14:34
- 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 <14218860+iulianbarbu@users.noreply.github.com>
@Kazy Kazy merged commit a03d051 into main Dec 4, 2023
31 of 33 checks passed
@Kazy Kazy deleted the setup-datadog branch December 4, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants