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

Renovate/GitHub.com cloudfy azuremonitor 38432 #38450

Open
wants to merge 9 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 <antoine@toulme.name>
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.

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
4 participants