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

ddtrace/tracer: support default origin on dynamic config to support Active Tracing telemetry spec #2623

Merged
merged 19 commits into from
May 30, 2024

Conversation

darccio
Copy link
Member

@darccio darccio commented Mar 20, 2024

What does this PR do?

Defaults dynamic config origin to default when created.

It also sets specific cases to env_var according to the telemetry API spec (more detail in AIT-8580).

Motivation

Fix failing telemetry system-tests in DataDog/system-tests#2063.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@darccio darccio requested a review from a team as a code owner March 20, 2024 16:48
@darccio darccio changed the title ddtrace/tracer: support default origin on dynamic config to support Active Tracing telemetry spec (re: AIT-8580) ddtrace/tracer: support default origin on dynamic config to support Active Tracing telemetry spec Mar 20, 2024
@darccio darccio requested a review from ahmed-mez March 20, 2024 16:50
@@ -334,7 +334,9 @@ func newConfig(opts ...StartOption) *config {
}
c.headerAsTags = newDynamicConfig("trace_header_tags", nil, setHeaderTags, equalSlice[string])
if v := os.Getenv("DD_TRACE_HEADER_TAGS"); v != "" {
WithHeaderTags(strings.Split(v, ","))(c)
c.headerAsTags.update(strings.Split(v, ","), originEnvVar)
Copy link
Contributor

@mtoffl01 mtoffl01 Mar 22, 2024

Choose a reason for hiding this comment

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

This just made me wonder whether the other settings that are available through RC should also get origin: originEnvVar on their dynamicConfig when enabled via env var, e.g, DD_TRACE_ENABLED
Maybe newDynamicConfig() should accept a value for origin, or maybe these we should also call dynamicConfig.update(val, originEnvVar) for these other options

Copy link
Member Author

Choose a reason for hiding this comment

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

@mtoffl01 You are right. I left this one as example and reminder to add more in a separate PR. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. Ideally the constructor should set the origin.

If for some reason we can't use the constructor to do so, the generic dynamicConfig type should provide a method to update the startup and current values and their origins instead of manipulating the fields directly in ddtrace/tracer/option.go.

Copy link
Contributor

@mtoffl01 mtoffl01 left a comment

Choose a reason for hiding this comment

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

I did leave one comment about potential changes, but those changes could be applied to a separate PR (since the title of this one explicitly states "support default origin" and my comment is for env_var origin)

@darccio
Copy link
Member Author

darccio commented Mar 25, 2024

@avivenzio-dd Before merging this one, what do you think it's the best course to ensure it's implementing right the spec?

@avivenzio-dd
Copy link

@avivenzio-dd Before merging this one, what do you think it's the best course to ensure it's implementing right the spec?

@darccio good call

Can we start up a small service and send some traces using the changes in this branch to a staging org? or is there a way to run the system tests based on this branch? I'm assuming sending some traces is probably easier.

We can verify the telemetry events are correct using their debug logs.

Copy link
Contributor

@ahmed-mez ahmed-mez left a comment

Choose a reason for hiding this comment

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

Looks good!
For future reference, I believe the dynamicConfig structure should be encapsulated in a separate package.
I saw we already bypass its method and access some field (e.g 25d3eb5) which is not the intended behavior (not thread-safe, error-prone and leaks the implementation details)

@@ -334,7 +334,9 @@ func newConfig(opts ...StartOption) *config {
}
c.headerAsTags = newDynamicConfig("trace_header_tags", nil, setHeaderTags, equalSlice[string])
if v := os.Getenv("DD_TRACE_HEADER_TAGS"); v != "" {
WithHeaderTags(strings.Split(v, ","))(c)
c.headerAsTags.update(strings.Split(v, ","), originEnvVar)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. Ideally the constructor should set the origin.

If for some reason we can't use the constructor to do so, the generic dynamicConfig type should provide a method to update the startup and current values and their origins instead of manipulating the fields directly in ddtrace/tracer/option.go.

@pr-commenter
Copy link

pr-commenter bot commented Apr 3, 2024

Benchmarks

Benchmark execution time: 2024-05-29 10:45:39

Comparing candidate commit aaae6a7 in PR branch dario.castane/AIT-9990-default-origin with baseline commit b913c87 in branch main.

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

scenario:BenchmarkHttpServeTrace-24

  • 🟥 execution_time [+391.964ns; +470.636ns] or [+2.378%; +2.856%]

scenario:BenchmarkStartSpanConcurrent-24

  • 🟥 execution_time [+433.012ns; +672.788ns] or [+6.254%; +9.717%]

@darccio darccio requested a review from a team as a code owner April 4, 2024 12:06
@darccio
Copy link
Member Author

darccio commented Apr 4, 2024

@avivenzio-dd The best way I found is to temporarily modify the GitHub CI config to run the parametric tests from DataDog/system-tests#2063. This week is heavy on ER thus reducing our capacity to do anything more sophisticated.

A first run shows that it's correctly running from the linked PR in system tests, combined with this PR. It failed anyway because we are returning default instead of env_var in a test, so I'm going to fix that I fixed it.

Edit: a second run seems to fail too but no longer due to the origin field's value. Please, can you check it? I think it's because you are not expecting the runtime ID as tag.

Copy link

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Stuck for more than 1 month label Apr 25, 2024
@avivenzio-dd
Copy link

@darccio is there something pending from my side on this? I think the tests should be all updated now.

@darccio
Copy link
Member Author

darccio commented Apr 25, 2024

@avivenzio-dd No, it's on my roof. Let me try to give it a try tomorrow.

@darccio darccio removed the stale Stuck for more than 1 month label Apr 29, 2024
@darccio darccio requested a review from a team as a code owner May 3, 2024 10:38
@darccio
Copy link
Member Author

darccio commented May 3, 2024

@avivenzio-dd I did another try but it fails. I'm having availability issues as I've been reassigned to a R&D project for one or two months. How do you want to proceed?

Copy link

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Stuck for more than 1 month label May 24, 2024
@darccio darccio requested a review from a team as a code owner May 28, 2024 14:50
Copy link
Contributor

@eliottness eliottness left a comment

Choose a reason for hiding this comment

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

Approved for appsec code 👍

@darccio darccio removed the stale Stuck for more than 1 month label May 29, 2024
@@ -33,7 +33,7 @@ jobs:
uses: actions/checkout@v3
with:
repository: 'DataDog/system-tests'
ref: ${{ inputs.ref }}
ref: kylev/enable-config-tests

Choose a reason for hiding this comment

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

looks like the tests from this branch are passing! I would just make sure to revert this before merging.

@darccio darccio merged commit 4e98120 into main May 30, 2024
193 of 200 checks passed
@darccio darccio deleted the dario.castane/AIT-9990-default-origin branch May 30, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants