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

[connector/datadog] Add config option for compute_top_level_by_span_kind #32836

Merged
merged 8 commits into from
May 6, 2024

Conversation

liustanley
Copy link
Contributor

Description:

Add config option for computing top level spans by span kind in the Datadog connector and exporter.

Link to tracking Issue:
#32005

Testing:

Documentation:

@liustanley liustanley requested review from mx-psi and songy23 as code owners May 2, 2024 21:05
@liustanley liustanley requested a review from a team May 2, 2024 21:05
@github-actions github-actions bot requested review from dineshg13 and mackjmr May 2, 2024 21:05
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

What would happen if I set compute_top_level_by_span_kind to true in the connector but not the exporter? What about the other way around?

.chloggen/stanley.liu_top-level-change-connector.yaml Outdated Show resolved Hide resolved
.chloggen/stanley.liu_top-level-change.yaml Outdated Show resolved Hide resolved
@liustanley
Copy link
Contributor Author

What would happen if I set compute_top_level_by_span_kind to true in the connector but not the exporter? What about the other way around?

compute_top_level_by_span_kind needs to be set wherever stats are being computed. By default, stats are computed in the connector but not the exporter, so this option needs to be set in the connector. If a customer is only using the exporter and disabled the noAPMStats feature gate, then this option would need to be set in the exporter. I plan on including this in the Datadog docs

@mx-psi
Copy link
Member

mx-psi commented May 3, 2024

If a customer is only using the exporter and disabled the noAPMStats feature gate, then this option would need to be set in the exporter. I

Oki, then I think we should allow users to set compute_top_level_by_span_kind on the exporter only if they have disabled the noAPMStats feature gate, and validate this during unmarshaling.

@liustanley
Copy link
Contributor Author

Oki, then I think we should allow users to set compute_top_level_by_span_kind on the exporter only if they have disabled the noAPMStats feature gate, and validate this during unmarshaling.

Sorry I just caught a mistake while testing. If we only set compute_top_level_by_span_kind on the Datadog connector, the exporter will still use the old top-level spans logic and doesn't set span metrics correctly. When using the connector, compute_top_level_by_span_kind needs to be set on both the connector and exporter. When only using the exporter, compute_top_level_by_span_kind needs to be set in addition to the noAPMStats feature gate being disabled.

I'm not sure if there's a way to enforce this across components, but I'll document this usage in the Datadog docs.

@liustanley liustanley requested review from mx-psi and songy23 May 3, 2024 20:02
@mx-psi
Copy link
Member

mx-psi commented May 6, 2024

Oki, then I think we should allow users to set compute_top_level_by_span_kind on the exporter only if they have disabled the noAPMStats feature gate, and validate this during unmarshaling.

Sorry I just caught a mistake while testing. If we only set compute_top_level_by_span_kind on the Datadog connector, the exporter will still use the old top-level spans logic and doesn't set span metrics correctly. When using the connector, compute_top_level_by_span_kind needs to be set on both the connector and exporter. When only using the exporter, compute_top_level_by_span_kind needs to be set in addition to the noAPMStats feature gate being disabled.

I'm not sure if there's a way to enforce this across components, but I'll document this usage in the Datadog docs.

@liustanley On this repository, I think we should mention this on

  • the release note
  • the configuration example
  • (optional) an info-level log on the connector

We don't have a good way to validate this cross-component requirement, but we should at least mention it widely.

@mx-psi mx-psi merged commit d444f48 into open-telemetry:main May 6, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants