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

[Azure Monitor Exporter] Add support to SpanEvents of type Exception #16333

Closed

Conversation

JorTurFer
Copy link

Description:
Even the specification defines how to treat exceptions, the azure monitor exporter doesn't implement the mapping between SpanEvents with name exception. To be able to use this exporter in our applications (and I guess that we are not the only with this requirement), we need to properly map the exceptions to the expected application insights table.
This PR adds support to it, processing the SpanEvents and generate the proper envelopes for events with event name exception

Link to tracking Issue: #16260

Testing: Following the already existing tests, I created a test to check that we don't create new envelops in there is any event attached to the span without the proper name and another test checking that in case of having the proper information, the envelope is generated with the correct information. I have also tested this on my on infrastructure to check that it works, but I can't reflect that in the PR (I could attach a picture, but it doesn't make sense IMO)

Documentation: I only added a note explaining that exception events are mapped to Application Insights Exceptions

Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
Copy link
Author

I'm not sure if I have to update the changelog with this change, I'd say yes, but I prefer to wait before doing it

@TylerHelmuth
Copy link
Member

Pinging @pcwiese as code owner

@pcwiese
Copy link
Contributor

pcwiese commented Nov 16, 2022

I was just polishing up a PR for mapping span events (including the Exceptions). If we go with this PR as a start, you need to do that piece also. Do you want to take that on?

@JorTurFer
Copy link
Author

I was just polishing up a PR for mapping span events (including the Exceptions). If we go with this PR as a start, you need to do that piece also. Do you want to take that on?

Depending on how much work you have already done xD I mean, if you need some weeks for having it ready, we can wait, if you need some months, we need these features :)
I'm willing to contribute with that features too. I plan to open another PR during next week, adding support to metrics.
If you tell me how do you want to map SpanEvents, I can add all of them in this PR if you want.

@JorTurFer
Copy link
Author

JorTurFer commented Nov 16, 2022

I mean, if you tell me the destination for events which aren't exceptions, I can generate the telemetry items and return them as part of the slice I added for exceptions, sending them to AI with current code

@JorTurFer
Copy link
Author

In fact, as part of this PR I mapped exceptions to AI.Exceptions and others to AI.CustomEvents and I undo the changes to map only Exceptions because I wasn't sure about CustomEvents
open-telemetry/opentelemetry-collector-contrib@654a14f (#16333)

Signed-off-by: Jorge Turrado <[email protected]>
@pcwiese
Copy link
Contributor

pcwiese commented Nov 16, 2022

I was actually nearly finished with the PR and was trying to get one out tomorrow. For metrics, I believe someone has a PR out , but I haven't taken a serious look yet. #14916

@JorTurFer
Copy link
Author

I was actually nearly finished with the PR and was trying to get one out tomorrow. For metrics, I believe someone has a PR out , but I haven't taken a serious look yet. #14916

Nice!, I can close this PR in that case. I started because I didn't see any in progress work, and we need this feature to migrating our apps to otel and I hadn't got any answer to the issue I opened (don't worry, I know how complex it to manage OSS projects, these things sometime happen).
So, if you are near to finish, I'll wait. One question, is there any support for CustomEvents incoming? I mean, how are you going to map other events with AI Telemetry items?

@JorTurFer
Copy link
Author

JorTurFer commented Nov 16, 2022

For our use case that Telemetry Item is needed, but I can't see anything in the specification about that, the closest things could be Logger.Events (Events API), but I'd like to know if these events will be added to CustomEvents or they will be more traces

@pcwiese
Copy link
Contributor

pcwiese commented Nov 16, 2022 via email

@JorTurFer
Copy link
Author

Thanks for the clarification.
I guess that we need to look for a workaround for customEvents, but at least you have given me a lot of useful information.
I close this PR (your branch looks so much better than mine xD) and I'll just wait. Let me know if I can help with something :)

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.

3 participants