-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: Add Sentry Exporter #565
Conversation
Codecov Report
@@ Coverage Diff @@
## master #565 +/- ##
==========================================
- Coverage 86.30% 86.22% -0.08%
==========================================
Files 191 195 +4
Lines 10388 10607 +219
==========================================
+ Hits 8965 9146 +181
- Misses 1099 1128 +29
- Partials 324 333 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
eecc5c7
to
eec7a40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AbhiPrasad, I can review the general structure and add some information but it will be good to get someone from Sentry to review the part of organizing spans in transactions and the translation itself.
|
||
sentrySpan := convertToSentrySpan(otelSpan, library, resourceTags) | ||
|
||
// If a span is a root span, we consider it the start of a Sentry transaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the transaction concept it will be good if someone from Sentry reviews this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've reviewed this in our own repo. See the PRs from: getsentry/opentelemetry-collector-contrib@master...getsentry:backup/sentryexporter
cc @rhcarvalho @bruno-garcia for another double check.
) | ||
|
||
// canonicalCodes maps OpenTelemetry span codes to Sentry's span status. | ||
// See numeric codes in https://godoc.org/github.com/open-telemetry/opentelemetry-proto/gen/go/trace/v1#Status_StatusCode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up. Do you think I should remove the functionality to read span.status
then in anticipation of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine to remove it later, we may do it whole sale.
828a0ab
to
9ed61e1
Compare
9ed61e1
to
8a669f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm vouching for this from the Sentry side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also vouch for this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @AbhiPrasad !
We've discussed changes as they were landing in the fork.
There we considered also the trade-offs and limitations (mainly due to the lack of support to individual span ingestion).
Happy to get this in and looking forward to get some feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Update README.md * Update processor/README.md Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> * Update processor/README.md Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> * Update processor/README.md Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Description:
This PR adds an exporter for Sentry (https://sentry.io/). I've tried my best to match the formatting and functionality of the other exporters, but please let me know if there is something I am missing.
Currently Sentry Exporter only supports exporting traces.
Testing:
I've written unit tests for 85% code coverage. I've also provided a demo repo that was used for more extensive testing - https://github.com/AbhiPrasad/opentelemetry-collector-sentry-demo
Documentation:
I've added a README in accordance with the exporters, as well as some documentation detailing the
OpenTelemetry
->Sentry
transformation.