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.

Sign up for GitHub

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

Jump to bottom

[translator/zipkin] stop dropping error tags in Zipkin translator #24547

Merged
merged 10 commits into from
Jul 28, 2023
Merged

[translator/zipkin] stop dropping error tags in Zipkin translator #24547

merged 10 commits into from
Jul 28, 2023
+126 −1

Conversation

SimunKaracic
Copy link
Contributor

Description:
Fixing a bug - Stop dropping error tags from Zipkin spans. The old code removes all errors from those spans, rendering them useless if an actual error happened. In addition, no longer delete error tags if they contain useful information. This now only deletes them if they are "true", and thus don't contain useful information. Also fixes the whole issue of

Link to tracking Issue: #16530

Testing: Absolutely none, I have written no Go code before this and have not even run the code.

I'm hoping CI saves me the trouble of installing and running everything locally

@SimunKaracic SimunKaracic requested a review from a team July 25, 2023 14:12
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 25, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from crobert-1 July 25, 2023 14:13
@SimunKaracic SimunKaracic changed the title Stop dropping error tags in Zipkin translator [translator/zipkin] stop dropping error tags in Zipkin translator Jul 25, 2023
@SimunKaracic SimunKaracic force-pushed the fix/16530-stop-removing-error-tags branch from 9eab4e7 to 1e18ea3 Compare July 25, 2023 14:14
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 25, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@SimunKaracic SimunKaracic force-pushed the fix/16530-stop-removing-error-tags branch from 1e18ea3 to beec384 Compare July 25, 2023 14:17
@SimunKaracic
Copy link
Contributor Author

SimunKaracic commented Jul 25, 2023 •
edited
Loading

I realise I'm dropping out of nowhere, but this is a simple issue that is completely blocking anyone who Zipkin format spans.
Please help me get it over the finish line

crobert-1
crobert-1 approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@crobert-1 crobert-1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This follows the original bug fix proposal from what I can tell, so it LGTM.

Also, I wrote some tests that failed with the original issue before your change, and now work as expected after. You're welcome to pull those in for this PR, and someone else can review those changes as well.

New tests added

crobert-1 and others added 2 commits July 26, 2023 10:48
@crobert-1 @SimunKaracic
[chore] [receiver/k8scluster] Remove dead code (open-telemetry#24535)
46e1e86 
As a result of the effort to move metrics in the k8s cluster receiver to
use the pdata format instead of OpenCensus, some helper methods are now
unused. This change removes unused code.
@atoulme @SimunKaracic
[chore] update codeowners to match the CODEOWNERS file (open-telemetr…
4867562 
…y#24454)

**Description:**
Follow up of open-telemetry#24404, some changes occurred in the last week or I had
misses in translating the CODEOWNERS file over.

Those minute changes fix that and prepare for the script to generate
`.github/CODEOWNERS`.
@SimunKaracic SimunKaracic force-pushed the fix/16530-stop-removing-error-tags branch from beec384 to 4867562 Compare July 26, 2023 08:48
@SimunKaracic SimunKaracic requested a review from a team July 26, 2023 08:48
@SimunKaracic SimunKaracic requested a review from jpkrohling as a code owner July 26, 2023 08:48
@github-actions github-actions bot added the receiver/zipkin Zipkin receiver label Jul 26, 2023
@SimunKaracic
Copy link
Contributor Author

Thank you so much @crobert-1 !
I've pulled in the tests, it should LGT everyone now 🚀

@SimunKaracic
Copy link
Contributor Author

Is there any way this can be included in the next release?
It would be of great help to be able to show errors in traces

@atoulme
Copy link
Contributor

atoulme commented Jul 27, 2023

Please add a changelog entry.

@SimunKaracic
Copy link
Contributor Author

Done! Is there a regular release cadence or something, so I know when to expect the release?

@crobert-1
Copy link
Member

Done! Is there a regular release cadence or something, so I know when to expect the release?

Releases are generally created every two weeks or so, but there isn't a set schedule. Here are all of the releases for historic context.

atoulme
atoulme approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM, I would ask a codeowner of the package to review as well.

@jpkrohling
Copy link
Member

@MovieStoreGuy @astencel-sumo, if you could review this within the next few hours, we can include it on this week's release.

One code owner has approved this already (@crobert-1), but it would still be good to have more approvals :-)

@jpkrohling jpkrohling merged commit 015d8db into open-telemetry:main Jul 28, 2023
@github-actions github-actions bot added this to the next release milestone Jul 28, 2023
@crobert-1 crobert-1 mentioned this pull request Oct 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crobert-1 crobert-1 crobert-1 approved these changes

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@atoulme atoulme atoulme approved these changes

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel andrzej-stencel is a code owner

@dmitryax dmitryax Awaiting requested review from dmitryax

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

Assignees

@dmitryax dmitryax

Labels
connector/exceptions examples/demo exporter/dataset exporter/opensearch internal/tools pkg/translator/zipkin receiver/k8scluster receiver/zipkin Zipkin receiver
Projects
None yet
Milestone
v0.82.0
Development

Successfully merging this pull request may close these issues.
6 participants
@SimunKaracic @atoulme @crobert-1 @jpkrohling @MovieStoreGuy @dmitryax