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

contrib/kafka: take env variable into account to enable DSM #2353

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

vandonr
Copy link
Contributor

@vandonr vandonr commented Nov 14, 2023

In most tracers, setting the DD_DATA_STREAMS_ENABLED env variable is enough to enable DSM, but in go this has to be done at wrapping time, which is inconsistent with other languages.

In this change, I deprecated the option to enable DSM, in favor of the env variable.

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • 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.

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.

@vandonr vandonr requested a review from a team November 14, 2023 14:59
@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Nov 14, 2023
@vandonr vandonr requested a review from rarguelloF November 14, 2023 15:06
@pr-commenter
Copy link

pr-commenter bot commented Nov 14, 2023

Benchmarks

Benchmark execution time: 2023-11-15 10:07:01

Comparing candidate commit 9cd3d19 in PR branch vandonr/kafka with baseline commit 5d0b85b in branch main.

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

Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

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

LGTM! just a comment about the deprecated options

Comment on lines 59 to 60
//
// Deprecated: Use the environment variable DD_DATA_STREAMS_ENABLED instead
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we keep both? I think its a common pattern in the Go tracer to provide both in-code and env variable choices for configurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, yes that's an argument that can be heard. Though it lets go users do stuff that won't work in other tracers, which is not the best, but being consistent within the go tracer itself is probably more important.

@vandonr
Copy link
Contributor Author

vandonr commented Nov 17, 2023

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 17, 2023

❌ MergeQueue

You are not allowed to use the merge queue towards main.

If you need support, contact us on slack #ci-interfaces with those details!

@vandonr
Copy link
Contributor Author

vandonr commented Nov 17, 2023

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 17, 2023

❌ MergeQueue

You are not allowed to use the merge queue towards main.

If you need support, contact us on slack #ci-interfaces with those details!

@dianashevchenko dianashevchenko enabled auto-merge (squash) November 17, 2023 13:02
@dianashevchenko dianashevchenko merged commit c654451 into main Nov 17, 2023
51 checks passed
@dianashevchenko dianashevchenko deleted the vandonr/kafka branch November 17, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants