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

Add flag to disable sending dependencies to telemetry #4497

Merged
merged 5 commits into from
Jan 27, 2023

Conversation

DDJavierSantos
Copy link
Contributor

@DDJavierSantos DDJavierSantos commented Jan 4, 2023

What Does This Do

Separating Dependencies from Telemetry Events, by disabling the DependencyService using a configuration flag.

Motivation

Currently, dependencies are sent if telemetry is enabled. The only way to stop sending dependencies was to fully disable telemetry. Introducing the DD_TELEMETRY_DEPENDENCY_COLLECTION_ENABLED (environment variable) or dd.telemetry.dependency-collection.enabled (system property) makes it possible to disable sending dependency metadata while keeping the rest of telemetry enabled.

Requirements:

  • Remove dependencies field from app-started
  • Only send dependencies-loaded event when DD_TELEMETRY_DEPENDENCY_COLLECTION_ENABLED flag is enabled
  • DD_TELEMETRY_DEPENDENCY_COLLECTION_ENABLED is enabled by default

Additional Notes

@DDJavierSantos DDJavierSantos requested a review from a team as a code owner January 4, 2023 11:31
@smola smola added the comp: telemetry Telemetry label Jan 4, 2023
@DDJavierSantos DDJavierSantos force-pushed the jsantos/disable_sending_dependencies branch from 1fe1458 to 86735fa Compare January 4, 2023 14:03
@devinsba
Copy link
Contributor

devinsba commented Jan 4, 2023

Maybe it would be good to give the customer the freedom to configure a set of artifact group IDs that they don't want to send? I agree that a flag to turn it off completely is good but a more fine grained approach might do enough to ease fears

Instrumentation inst = Mock()

void setup(){
injectSysConfig("dd.telemetry.send.dependencies", "false")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe dd.telemetry.dependency-collection.enabled would be better aligned with other feature enable/disable configs

Copy link
Member

@smola smola Jan 17, 2023

Choose a reason for hiding this comment

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

Wait a sec. This setting comes from a spec that will be the same across languages. So I would rather not diverge from it. Bringing it up with the spec authors.

cc @DDJavierSantos

Copy link
Contributor

Choose a reason for hiding this comment

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

@smola I can't find anything resembling that setting when searching through our documents. Naming is hard, and it would be good if external facing user configuration followed some convention going forward.

Copy link
Member

Choose a reason for hiding this comment

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

The spec was updated to use DD_TELEMETRY_DEPENDENCY_COLLECTION_ENABLED.
@DDJavierSantos We're good to update the PR to DD_TELEMETRY_DEPENDENCY_COLLECTION_ENABLED / dd.telemetry.dependency-collection.enabled.

@smola
Copy link
Member

smola commented Jan 5, 2023

@devinsba Maybe it would be good to give the customer the freedom to configure a set of artifact group IDs that they don't want to send? I agree that a flag to turn it off completely is good but a more fine grained approach might do enough to ease fears

Good point. I'll bring this up with the telemetry team.

@smola smola changed the title Jsantos/disable sending dependencies Add flag to disable sending dependencies to telemetry Jan 5, 2023
@DDJavierSantos DDJavierSantos merged commit 10eb3a7 into master Jan 27, 2023
@DDJavierSantos DDJavierSantos deleted the jsantos/disable_sending_dependencies branch January 27, 2023 09:28
@github-actions github-actions bot added this to the 1.7.0 milestone Jan 27, 2023
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.

6 participants