-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[exporter/azuremonitorexporter] fix: correct parent for span events #30659
[exporter/azuremonitorexporter] fix: correct parent for span events #30659
Conversation
@@ -133,6 +133,7 @@ func spanToEnvelopes( | |||
} | |||
|
|||
spanEventEnvelope := newEnvelope(span, toTime(spanEvent.Timestamp()).Format(time.RFC3339Nano)) | |||
spanEventEnvelope.Tags[contracts.OperationParentId] = traceutil.SpanIDToHexOrEmptyString(span.SpanID()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newEnvelope
method has the entry for envelope.Tags[contracts.OperationParentId]
, that seems to be correct.
envelope.Tags[contracts.OperationParentId] = traceutil.SpanIDToHexOrEmptyString(span.ParentSpanID())
spanEventEnvelope.Tags[contracts.OperationParentId] = traceutil.SpanIDToHexOrEmptyString(span.SpanID())
Setting the current span ID as the parent does not seem correct. Did you validate with an app that issue #27233 is resolved by this update?
Providing the .NET implementation here for reference - https://github.com/Azure/azure-sdk-for-net/blob/2d1996d14d430c541c5beabad612b98a01fb4a4e/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/TelemetryItem.cs#L19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajkumar-rangaraj - the nifty difference here - we are not mapping spans, we are mapping logs - e.g. compare with https://github.com/Azure/azure-sdk-for-java/blob/9e6bab9bb50d865ed8b4e5ab3198e3fc56d9e468/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/implementation/LogDataMapper.java#L186 and lines around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct in the context of logs. All logs will have the parent ID labeled as 'spanid.' However, you are modifying the trace envelope, which corresponds to spans. Logs are handled differently; therefore, the change should be applied to the log exporter file. The current proposed change might break the UI in other scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving your changes to log_to_envelope.go
should resolve an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajkumar-rangaraj - first huge sorry - with the LogDataMapper
I've pointed you to the wrong direction. We are still and only talking about trace data. However I've found the more correct line that underpins my change also in the azure-sdk-for-java
- https://github.com/Azure/azure-sdk-for-java/blob/9e6bab9bb50d865ed8b4e5ab3198e3fc56d9e468/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/implementation/SpanDataMapper.java#L779
For Event
s the parent span is the span of the current operation. That also makes sense, because the events are just annotations of the current operation/tracing span. So maybe we should take one step back, and find out how far we already agree.
- There is a problem with the original implementation (azuremonitorexporter assign wrong parentId to span events #27233) - the original behaviour could and should be improved?
- The provided PR solves this issue - and has no unintended side effects?
- Is the form of the changes okay, or should the patch be rephrased?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. It makes sense for span events.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@pcwiese, Could you please review this PR. |
@alaendle Could you please fix this one? |
@open-telemetry/collector-contrib-approvers Could you please approve the pipeline and help merge this PR. It has got an approval from the code owner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a changelog
62185d2
to
d4db28c
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Pinging @djaglowski - maybe you could approve/merge this PR? Or is still something missing? |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This is becoming more and more of a frustrating experience. The PR is now 3 month old and I still can't predict whether or when he will make it to the finish line. |
Apologies @alaendle. I understand how this can be frustrating. I see we have codeowner and approver review already. I went ahead and stuck the |
Description: Corrected parent for exported span events.
For details please take a look at the corresponding issue.
I think the change is pretty straight forward - one line added.
However I'm not a go-expert - so there might exist a more elegant implementation.
Link to tracking Issue: #27233
Testing: Adapted existing units test.
Documentation: n/a
(fyi @pcwiese)