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

[PROF-7786] Include _dd.profiling.enabled tag #2913

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jun 19, 2023

What does this PR do?:

This PR implements the Datadog-internal
"RFC: Identifying whether a span’s host has profiling enabled" similar to what
dd-trace-java does by including a _dd.profiling.enabled tag when profiling is enabled.

This is similar to the existing _dd.appsec.enabled tag.

Motivation:

This enables a better user experience in the "Code Hotspots" tab, e.g. for users that run profiling in a subset of their fleet.

This way we can immediately tell them "hey, there's no profiling data" rather than making them wait a few minutes and then telling them that we didn't find anything.

Additional Notes:

N/A

How to test the change?:

Validate that the _dd.profiling.enabled tag shows up in the UX.

**What does this PR do?**:

This PR implements the Datadog-internal
"RFC: Identifying whether a span’s host has profiling enabled"
similar to what
[dd-trace-java](DataDog/dd-trace-java#5395)
does by including a `_dd.profiling.enabled` tag when profiling is
enabled.

This is similar to the existing `_dd.appsec.enabled` tag.

**Motivation**:

This enables a better user experience in the "Code Hotspots" tab,
e.g. for users that run profiling in a subset of their fleet.

This way we can immediately tell them "hey, there's no profiling
data" rather than making them wait a few minutes and then telling
them that we didn't find anything.

**Additional Notes**:

N/A

**How to test the change?**:

Validate that the `_dd.profiling.enabled` tag shows up in the UX.
@ivoanjo ivoanjo requested a review from a team June 19, 2023 15:51
Comment on lines 454 to 455
profiling_enabled:
!!(defined?(Datadog::Profiling) && Datadog::Profiling.respond_to?(:enabled?) && Datadog::Profiling.enabled?), # rubocop:disable Style/DoubleNegation
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be checked earlier and cached carefully if possible.

In today's world, I believe we can move this into the Tracing::Tracer object initialization process with no loss of correctness, then pass the value here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Fixed in cbe0317.

I had to do it lazily instead of during initialization to avoid any potential of loops (Profiling::Component taking an optional_tracer and doing something similar in the other direction), but otherwise it was a straightforward change.

@github-actions github-actions bot added the core Involves Datadog core libraries label Jun 20, 2023
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

A bit of coupling, but our products are coupled a bit at the end of day.

@ivoanjo
Copy link
Member Author

ivoanjo commented Jun 22, 2023

Thanks Marco! I hope the trade-off for improving the user experience for the "Code Hotspots" when looking at at a trace is worth it. Especially since we're going to bring a lot of really cool features to the UX next -- stay tuned :)

@ivoanjo ivoanjo merged commit 3bd8980 into master Jun 22, 2023
@ivoanjo ivoanjo deleted the ivoanjo/tag-profiling-enabled branch June 22, 2023 07:58
@github-actions github-actions bot added this to the 1.13.0 milestone Jun 22, 2023
@GustavoCaso
Copy link
Member

@ivoanjo @marcotc Maybe profiling could take a similar approach as ASM for adding tags in a way that is not as a couple.

Could the profiler code append the tag outside the code of tracing?

@ivoanjo
Copy link
Member Author

ivoanjo commented Jun 22, 2023

Could the profiler code append the tag outside the code of tracing?

Not right now, because the profiler never gets called by the app, as it never instruments anything.

You'll never see a profiler method sandwiched in between customer code, like you do for tracing/asm; it only sits in the background as a periodic observer.

@delner
Copy link
Contributor

delner commented Jun 27, 2023

I think we need to revisit this. I don't think this is acceptable in its current state, but I want to review with stakeholders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants