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

Fix profiler config propagation #1403

Merged
merged 1 commit into from Jun 11, 2021
Merged

Conversation

Qard
Copy link
Collaborator

@Qard Qard commented Jun 3, 2021

The profiler configs are not currently propagated correctly, so it's impossible to use a non-localhost agent. This fixes that and better encapsulates config handling within the profiler rather than in tracer code.

@Qard Qard added the profiling label Jun 3, 2021
@Qard Qard requested a review from a team as a code owner June 3, 2021 01:28
@Qard Qard force-pushed the fix-profiler-config-propagation branch 2 times, most recently from 3af3242 to c244e46 Compare June 3, 2021 20:08
@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #1403 (4651463) into master (1e9906d) will increase coverage by 0.03%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1403      +/-   ##
==========================================
+ Coverage   94.24%   94.28%   +0.03%     
==========================================
  Files         155      155              
  Lines        6275     6282       +7     
==========================================
+ Hits         5914     5923       +9     
+ Misses        361      359       -2     
Impacted Files Coverage Δ
packages/dd-trace/src/profiling/config.js 97.67% <95.00%> (-2.33%) ⬇️
packages/datadog-plugin-router/src/index.js 82.43% <100.00%> (-0.69%) ⬇️
packages/dd-trace/src/profiler.js 58.33% <100.00%> (-6.67%) ⬇️
packages/dd-trace/src/profiling/exporters/agent.js 97.36% <100.00%> (ø)
...kages/datadog-plugin-cassandra-driver/src/index.js 97.93% <0.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7706731...4651463. Read the comment docs.

Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

The idea behind how this is currently written is to expose the profiler with a public API that would match what would be exposed to users. Right now one of the problems of the tracer is that it's very clunky to configure because nothing is exposed, so everything must be a configuration option even when it makes no sense and would be better served by a programmatic API. For example, 2 different exporters could have completely different available options, and that is confusing to configure with only configuration options.

Any way to fix the issue while ensuring that we can still expose a public API that makes the most sense from a usability perspective?

@Qard
Copy link
Collaborator Author

Qard commented Jun 6, 2021

I'm not really sure what you mean by that. Logically, this change is no different from the current code other than just passing through the url, hostname, and port of the agent. The other restructuring is functionally identical as long as it remains directly connected to the tracer code but just isolates it a bit better which makes it easier to possibly extract the profiler to another module in the future, which may be required for the shared native module we are building.

@rochdev
Copy link
Member

rochdev commented Jun 7, 2021

The other restructuring is functionally identical as long as it remains directly connected to the tracer code but just isolates it a bit better which makes it easier to possibly extract the profiler to another module in the future, which may be required for the shared native module we are building.

Yes but that was precisely the point of exposing the exporters directly, although that may actually be a lower-level package later on instead. I will approve since for now it doesn't matter anyway and the change is minimal.

rochdev
rochdev previously approved these changes Jun 7, 2021
@Qard Qard force-pushed the fix-profiler-config-propagation branch from 4e003d6 to 4651463 Compare June 7, 2021 21:21
@Qard Qard merged commit 7a9c335 into master Jun 11, 2021
@Qard Qard deleted the fix-profiler-config-propagation branch June 11, 2021 20:56
bengl pushed a commit that referenced this pull request Jul 6, 2021
bengl pushed a commit that referenced this pull request Jul 6, 2021
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.

None yet

2 participants