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

Define profiling.instrumentation.source Log Message Attribute #336

Conversation

tduncan
Copy link

@tduncan tduncan commented Feb 17, 2025

The profiling backend ingestion process needs to be able to distinguish between callstacks collected by the existing continuous "Always On" profiler and those collected by the upcoming trace snapshot profiler. This PR defines an attribute to be included in a log message body that will identify which profiling process was responsible for collecting and reporting the exported callstacks.

Copy link
Contributor

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Can you please add a changelog entry?

@tduncan
Copy link
Author

tduncan commented Feb 17, 2025

Can you please add a changelog entry?

Done.

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

I am not against change for a short term solution, but we need to start thinking how to handle the OTLP profiling protocol. I hope it will go live this year.

In such cases you will be not able to expect such attributes.
For known, libraries, you can probably utilize

Comment on lines +132 to +134
- `profiling.instrumentation.source` MUST be set to either:
- `continuous` for continuous "Always On" profiling
- `snapshot` for trace snapshot profiling
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that, for backward compatibility, you should mention that it will be treated as continuous.

Copy link
Contributor

Choose a reason for hiding this comment

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

This, there are too many existing installations, my suggestion is that profiling.instrumentation.source must only be set when anything other than continuous is used (as the backend treats all of the profiles as continuous by default).

@tduncan
Copy link
Author

tduncan commented Feb 18, 2025

I am not against change for a short term solution, but we need to start thinking how to handle the OTLP profiling protocol. I hope it will go live this year.

In such cases you will be not able to expect such attributes. For known, libraries, you can probably utilize

SampleType and PeriodType would seem to be describing the data itself, where as what we need to know is information about the context from which the callstacks were collected as the values for both SampleType and PeriodType would be identical. The shape of the profiling data does not vary between the existing continuous profiling solution and the trace snapshot extension we're porting into the Java agent.

I was hoping to use InstrumentationScope but conversations with individuals more familiar with the GDI process made it clear that, at least for now, the InstrumentationScope information will not be available for us to rely on.

tduncan and others added 6 commits February 18, 2025 14:57
…cted a given callstack.

Signed-off-by: thomasduncan <tduncan@splunk.com>
…ion.

Signed-off-by: thomasduncan <tduncan@splunk.com>
…ribute and its values.

Signed-off-by: thomasduncan <tduncan@splunk.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Signed-off-by: thomasduncan <tduncan@splunk.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Signed-off-by: thomasduncan <tduncan@splunk.com>
Signed-off-by: thomasduncan <tduncan@splunk.com>
@tduncan tduncan force-pushed the add-profiling-instrumentation-source-attribute branch from fb509bb to facce30 Compare February 18, 2025 23:01
@Kielek
Copy link
Contributor

Kielek commented Feb 19, 2025

I am not against change for a short term solution, but we need to start thinking how to handle the OTLP profiling protocol. I hope it will go live this year.
In such cases you will be not able to expect such attributes. For known, libraries, you can probably utilize

SampleType and PeriodType would seem to be describing the data itself, where as what we need to know is information about the context from which the callstacks were collected as the values for both SampleType and PeriodType would be identical. The shape of the profiling data does not vary between the existing continuous profiling solution and the trace snapshot extension we're porting into the Java agent.

I was hoping to use InstrumentationScope but conversations with individuals more familiar with the GDI process made it clear that, at least for now, the InstrumentationScope information will not be available for us to rely on.

For our internal internal profiling format, we can put any data we want. The problem will be with the OTLP-profiling of the protocol. From the very beginning we should be ready to understand different sets of data. The most important bringing from OTEL world is ebpf-profiling. What is more, for sure the .NET data will be send in the OTel-compliant way. The data source is working, we need to add only the exporter.

Avoiding any Cisco/Splunk specific attributes should be our goal. If needed, we can start working on semantic-convention for profiling, but need to have good proposal for this.

@tduncan
Copy link
Author

tduncan commented Feb 19, 2025

I've mangled the commit signing and unable to figure out how to fix it. Instead I've opened #337 with correctly signed commits.

@tduncan tduncan closed this Feb 19, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants