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

When batching, same object is mutated destroying exception information #1187

Merged
merged 4 commits into from Feb 8, 2023

Conversation

calleo
Copy link
Contributor

@calleo calleo commented Feb 6, 2023

While testing Azure Exporter to see if exceptions (and traces) where propagated correctly to Application Insights I noticed that stack traces ended upp in a custom field called "stacktrace" but they where not tagged as exceptions in Application Insights.

Each time an exception occurs within a span, the function span_data_to_envelope will yield twice. The problem is that the same envelope object is re-used and modified. The current tests won't fail because each yield is tested in isolation, given the nature of next.

In my setup, emit is always called with a batch of span's, meaning all of them are processed before sent to Application Insights. As a result I end up with two envelopes of type Microsoft.ApplicationInsights.Request. I would expect one of type Microsoft.ApplicationInsights.Exception and one of Microsoft.ApplicationInsights.Request.

@google-cla
Copy link

google-cla bot commented Feb 6, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@calleo calleo marked this pull request as ready for review February 6, 2023 20:29
@calleo calleo mentioned this pull request Feb 6, 2023
.python-version Outdated Show resolved Hide resolved
@lzchen
Copy link
Contributor

lzchen commented Feb 6, 2023

Good catch, please fix the build errors and then we can merge this.

@calleo calleo force-pushed the master branch 2 times, most recently from cb75eb7 to 211fa74 Compare February 7, 2023 09:47
@calleo
Copy link
Contributor Author

calleo commented Feb 7, 2023

Good catch, please fix the build errors and then we can merge this.

Thanks @lzchen. Updated the way the object is copied, so hopefully works now. Had some unrelated tests fail locally (prometheus extension), but I believe something is not properly setup in my environment.

@lzchen lzchen closed this Feb 7, 2023
@lzchen lzchen reopened this Feb 7, 2023
@lzchen
Copy link
Contributor

lzchen commented Feb 7, 2023

@calleo
Please rebase from latest master to get the builds running :)

@calleo calleo force-pushed the master branch 2 times, most recently from 684ab6a to c710688 Compare February 7, 2023 20:53
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

@calleo
Would you kindly add a CHANGELOG entry?

@calleo
Copy link
Contributor Author

calleo commented Feb 8, 2023

@calleo Would you kindly add a CHANGELOG entry?

Thanks' for reviewing. I've added this fix to the changelog now.

@lzchen lzchen merged commit b85e476 into census-instrumentation:master Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants