Skip to content

Add flag to disable sending dependencies to telemetry#4497

Merged
DDJavierSantos merged 5 commits into
masterfrom
jsantos/disable_sending_dependencies
Jan 27, 2023
Merged

Add flag to disable sending dependencies to telemetry#4497
DDJavierSantos merged 5 commits into
masterfrom
jsantos/disable_sending_dependencies

Conversation

@DDJavierSantos

@DDJavierSantos DDJavierSantos commented Jan 4, 2023 •
edited by smola
Loading

Copy link
Copy Markdown
Contributor

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
smola
smola requested changes Jan 4, 2023
View reviewed changes
Comment thread dd-trace-api/src/main/java/datadog/trace/api/config/GeneralConfig.java Outdated
Comment thread telemetry/src/test/groovy/datadog/telemetry/DisabledDependencyServiceTest.groovy Outdated
ValentinZakharov
ValentinZakharov reviewed Jan 4, 2023
View reviewed changes
Comment thread telemetry/src/main/java/datadog/telemetry/TelemetrySystem.java Outdated
@DDJavierSantos
DDJavierSantos force-pushed the jsantos/disable_sending_dependencies branch from 1fe1458 to 86735fa Compare January 4, 2023 14:03
@devinsba

devinsba commented Jan 4, 2023

Copy link
Copy Markdown
Contributor

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

devinsba
devinsba reviewed Jan 4, 2023
View reviewed changes
Comment thread telemetry/src/test/groovy/datadog/telemetry/DisabledDependencyServiceTest.groovy Outdated
Instrumentation inst = Mock()

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

@devinsba devinsba Jan 4, 2023

Copy link
Copy Markdown
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

@smola smola Jan 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

@bantonsson bantonsson Jan 19, 2023

Copy link
Copy Markdown
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.

@smola smola Jan 27, 2023

Copy link
Copy Markdown
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

smola commented Jan 5, 2023

Copy link
Copy Markdown
Member

@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
mcculls
mcculls reviewed Jan 6, 2023
View reviewed changes
Comment thread telemetry/src/main/java/datadog/telemetry/TelemetrySystem.java Outdated
@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

Reviewers

@bantonsson bantonsson bantonsson left review comments
@smola smola smola approved these changes
@mcculls mcculls mcculls approved these changes
@ValentinZakharov ValentinZakharov ValentinZakharov approved these changes
@devinsba devinsba Awaiting requested review from devinsba

Assignees

No one assigned

Labels

comp: telemetry Telemetry

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

6 participants

@DDJavierSantos @devinsba @smola @mcculls @bantonsson @ValentinZakharov