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

Report integration and integration_version to telemetry #82

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

dmehala
Copy link
Collaborator

@dmehala dmehala commented Dec 20, 2023

Description

This PR send a new type of payload app_integrations_change to Telemetry v2. This new payload allows use know which integration is using the Tracer.

@dmehala dmehala changed the title [wip] Report integration to telemetry [wip] Report product and product version to telemetry Dec 20, 2023
@dgoffredo
Copy link
Contributor

Each of our "integrations" will need separate consideration for how the designation will make it into dd-trace-cpp.

Here are the ones that come to mind:

  • nginx-datadog we can do what we want, since we control the code. A TracerConfig field will work just fine.
  • envoy we also have programmatic control over, so we can use TracerConfig, but we have to think of how to handle
  • istio, which uses envoy via a special envoy build called istio/proxy, which then gets rolled into images and mentioned in kubernetes YAML configs by istio proper. Not sure how best to get the "istio-ness" into dd-trace-cpp, since Istio isn't handling the TracerConfig like envoy. Perhaps it's best to add an optional configuration parameter to Envoy's YAML, and have Istio setups default the value when Datadog tracing is mentioned. Tricky.
  • ingress-nginx, which previously used dd-opentracing-cpp but now is OpenTelemetry-over-Datadog-Agent-only, until I finish the "OpenTelemetry API" projects. If a customer is using the OpenTelemetry way, then we don't have to worry about "integration," because dd-trace-cpp is not involved. If a customer is using a version with dd-opentracing-cpp, then same thing. The question is how ingress-nginx will tell its OpenTelemetry nginx module that the Datadog implementation ought to designate itself as ingress-nginx 🤔 . That's a tough one.
  • Your Apache POC can use TracerConfig, no problem.
  • Kong doesn't use dd-trace-cpp.

Maybe an environment variable. Doesn't feel right, though.

Have a think about it.

@dmehala
Copy link
Collaborator Author

dmehala commented Dec 21, 2023

Maybe an environment variable. Doesn't feel right, though.

Agree, it's a nice workaround but it does not feel appropriate to use an environment variable. It raises concerns about relying on a value that can be changed arbitrarily and depending on users to set it accurately. Part of it can probably be mitigated with single-step APM instrumentation.

Back to our integration challenges, the underlying issue is essentially about propagating the value from X to dd-trace-cpp. As a former colleague used to say, there are no problems we can't solve without another level of indirection.

istio integration

I had a quick look on the relationship between envoy and istio. I started my journey on a naive thought, at some point istio needs to inform envoy which tracer to use.

I stumbled upon istio/pilot/pkg/networking/core/v1alpha3/tracing.go, it can return a datadogConfig. This DatadogConfig is then used to set up our Tracer.

A potential solution might involve adding a field to datadog.proto and seting that field in istio code.

ingress-nginx

No idea 🤷‍♂️ Further investigation is required. Let's suppose ingress-nginx uses dd-trace-cpp, in my opinion, it is better to have false positive telemetry data (ingress-nginx tagged as nginx) than nothing.

@dmehala dmehala force-pushed the dmehala/telemetry-add-integration branch from 1dabbc0 to 0dad300 Compare January 10, 2024 13:31
@dmehala dmehala marked this pull request as ready for review January 10, 2024 13:31
@dmehala dmehala changed the title [wip] Report product and product version to telemetry Report integration and integration_version to telemetry Jan 10, 2024
@dmehala dmehala requested a review from dgoffredo January 10, 2024 13:33
@pr-commenter
Copy link

pr-commenter bot commented Jan 10, 2024

Benchmarks

Benchmark execution time: 2024-01-11 15:44:04

Comparing candidate commit 022a8da in PR branch dmehala/telemetry-add-integration with baseline commit 17098c1 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

Copy link
Contributor

@dgoffredo dgoffredo left a comment

Choose a reason for hiding this comment

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

As we discussed in a recent meeting, once you figure out whether there's a difference between:

  • the absence of the integration fields
  • the presence of the integration fields, but as empty strings

then we can finalize the policy about TracerConfig::integration_*, document them, and be done with this.

The code and test look good.

@dmehala
Copy link
Collaborator Author

dmehala commented Jan 11, 2024

As we discussed in a recent meeting, once you figure out whether there's a difference between:

  • the absence of the integration fields
  • the presence of the integration fields, but as empty strings

The version field is not mandatory in the telemetry payload, an empty string as the same behaviour as if the field is not there. (confirmed by telemetry team)

I tried to document those fields: 3ba3d6c

Copy link
Contributor

@dgoffredo dgoffredo left a comment

Choose a reason for hiding this comment

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

Nicely done.

src/datadog/tracer_config.h Outdated Show resolved Hide resolved
Co-authored-by: David Goffredo <david.goffredo@datadoghq.com>
@dmehala dmehala merged commit 89a8e36 into main Jan 11, 2024
19 checks passed
@dmehala dmehala deleted the dmehala/telemetry-add-integration branch January 11, 2024 15:48
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.

2 participants