-
Notifications
You must be signed in to change notification settings - Fork 0
Add Sentry to adservice #15
Conversation
src/adservice/Dockerfile
Outdated
|
|
||
| COPY --from=builder /usr/src/app/ ./ | ||
| ADD https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/download/v$version/opentelemetry-javaagent.jar /app/opentelemetry-javaagent.jar | ||
| ADD https://search.maven.org/remotecontent?filepath=io/sentry/sentry-opentelemetry-agent/6.9.1/sentry-opentelemetry-agent-6.9.1.jar /app/opentelemetry-javaagent.jar |
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.
The way I did it here only uses Sentry to pass OTEL Spans to Sentry. There's no errors here. Do we want to
- change this (maybe another PR) to initialize Sentry in the target application and have errors showcased as well?
- have another service written in Java that showcases this combined use case of having OTEL for instrumentation and Sentry for errors
I like how this hardly differs from the OTEL sample in that you only replace the agent JAR and set DSN and tracesSampleRate and you're good to go with OTEL -> Sentry instrumentation.
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.
change this (maybe another PR) to initialize Sentry in the target application and have errors showcased as well?
I think we initialize Sentry for errors in other services, so yes, let's maybe do it in a separate PR later (to be confirmed with the team if we want to do it right away).
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 should initialize Sentry errors as well - we want this to be an example of what a user will do in production.
have another service written in Java that showcases this combined use case of having OTEL for instrumentation and Sentry for errors
Let's have this as a stretch goal for after we make the next release of the otel java sdk, maybe we can also think about contributing the service upstream. Maybe we write it in Kotlin then to make it more likely we merge it in?
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 was thinking about adding another service that uses Spring Boot. I could also do it in Kotlin (it's what I used in the past) but I guess the majority of people still use Spring with Java and not Kotlin. But I guess anywhone familiar with Java can easily understand most of the Kotlin code without looking anything up.
I'll update this example before merging this PR. Then after I added another service I can show how to use Sentry for errors and OTEL for instrumentation in the other sample and revert this one to what it is now (a showcase of allowing for a drop-in replacement of the original OTEL agent).
src/adservice/Dockerfile
Outdated
|
|
||
| COPY --from=builder /usr/src/app/ ./ | ||
| ADD https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/download/v$version/opentelemetry-javaagent.jar /app/opentelemetry-javaagent.jar | ||
| ADD https://search.maven.org/remotecontent?filepath=io/sentry/sentry-opentelemetry-agent/6.9.1/sentry-opentelemetry-agent-6.9.1.jar /app/opentelemetry-javaagent.jar |
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 should initialize Sentry errors as well - we want this to be an example of what a user will do in production.
have another service written in Java that showcases this combined use case of having OTEL for instrumentation and Sentry for errors
Let's have this as a stretch goal for after we make the next release of the otel java sdk, maybe we can also think about contributing the service upstream. Maybe we write it in Kotlin then to make it more likely we merge it in?
Changes
This replaces the OpenTelemetry Java Agent with the Sentry one and also sets DSN and tracesSampleRate.
Merge Requirements
For new features contributions please make sure you have completed the following
essential items:
CHANGELOG.mdupdated to document new feature additionsMaintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.