Skip to content

Renovate/GitHub.com cloudfy azuremonitor 38432 #38450

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cloudfy
Copy link

@cloudfy cloudfy commented Mar 7, 2025

Description

This PR attempts to address a potential solution for issue #38432 by handling the state of logRecord when compared to the semantic model. Its my first PR to this project, please comments for feedback.

Link to tracking issue

Fixes: #38432

Testing

make

Documentation

Azure Monitor Exporter documentation is updated to reflect and include the description of exceptions handling.

@cloudfy cloudfy requested a review from a team as a code owner March 7, 2025 10:02
@cloudfy cloudfy requested a review from mwear March 7, 2025 10:02
Copy link

linux-foundation-easycla bot commented Mar 7, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested review from hgaol and pcwiese March 7, 2025 10:02
@cloudfy cloudfy force-pushed the renovate/GitHub.com-cloudfy-azuremonitor-38432 branch from 9f3be64 to e683e4c Compare March 7, 2025 10:17
@cloudfy cloudfy closed this Mar 7, 2025
@cloudfy cloudfy force-pushed the renovate/GitHub.com-cloudfy-azuremonitor-38432 branch from e683e4c to 743887c Compare March 7, 2025 10:25
@cloudfy cloudfy reopened this Mar 7, 2025
@atoulme
Copy link
Contributor

atoulme commented Mar 7, 2025

CI will fail - see the cues in the build to address immediate issues. Thanks for the PR!

Co-authored-by: Antoine Toulme <[email protected]>
Copy link
Author

@cloudfy cloudfy left a comment

Choose a reason for hiding this comment

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

Reviewed the code review comments and addressed new review.

@hgaol
Copy link
Member

hgaol commented Mar 11, 2025

Thank you for your contributions! Since this change will affect current behaviour, my general feeling is to align with azure-sdk. + @pcwiese to take a look.

@cloudfy
Copy link
Author

cloudfy commented Mar 11, 2025

@pcwiese - would live your view on this, and must admit @hgaol is right, behaviour will change as exception traces will be moved into exceptions table.

If a limiting factor is important, might solve the breaking change by using a configuration parameter?

Love you feedback, @hgaol thanks for highlight.

@hgaol
Copy link
Member

hgaol commented Mar 11, 2025

Hi @cloudfy , here's the implementation in Azure Monitor OpenTelemetry Exporter .NET SDK for your reference. I think we'd better to align about handling log exception.

https://github.com/Azure/azure-sdk-for-net/blob/a708aadd8428d8cdf37a6d206683ac7599c00de1/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/LogsHelper.cs#L65

@cloudfy
Copy link
Author

cloudfy commented Mar 11, 2025

@hgaol exactly - we've used the Azure SDK for logging before, but have to move to collector due to load. The collector however does not handle exceptions as the Azure SDK, which is why the PR (to align). I'll leave the team up to decide, but it does introduce a breaking change by handling exceptions into the right table.

@atoulme
Copy link
Contributor

atoulme commented Mar 17, 2025

Please address conflicts and mark ready to review again.

@atoulme atoulme marked this pull request as draft March 17, 2025 21:40

This exporter saves exception records to Application Insights `exceptions` table when log records indicate an excetion [specification](https://opentelemetry.io/docs/specs/otel/trace/exceptions/).

Exceptions must be log records with `SeverityNumber` of 17 (Error) and the following attributes MUST be filled out:
Copy link
Member

Choose a reason for hiding this comment

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

pls update based on current implementation

@hgaol
Copy link
Member

hgaol commented Mar 18, 2025

Pls add a switch config to enable this feature or not. And use false by default to avoid involving breaking change for existing users.

@hgaol
Copy link
Member

hgaol commented Mar 18, 2025

Copy link
Contributor

github-actions bot commented Apr 1, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 1, 2025
@pableess
Copy link

This would be a welcome change for my organization as well since we moved from using the Azure SDK to now using the collector.

@github-actions github-actions bot removed the Stale label Apr 11, 2025
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 26, 2025
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.

[azuremonitorexporter] LogRecordToEnvelope does not handle exceptions
5 participants