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

Initialize Sentry in OTEL Java Agent and allow configuring it #2386

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Nov 23, 2022

#skip-changelog

📜 Description

Can configure as usual (env vars and props file)

💡 Motivation and Context

To not have to add Sentry to the actual application but be able to initialize and configure inside the Agent.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 52d86a3

@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 342.02 ms 385.78 ms 43.75 ms
Size 1.73 MiB 2.32 MiB 610.45 KiB

Previous results on branch: feat/allow-configuring-sentry-otel-agent

Startup times

Revision Plain With Sentry Diff
b7848db 253.74 ms 339.94 ms 86.19 ms
93fc323 427.42 ms 486.69 ms 59.27 ms
b1066e6 294.94 ms 308.87 ms 13.93 ms

App size

Revision Plain With Sentry Diff
b7848db 1.73 MiB 2.32 MiB 610.45 KiB
93fc323 1.73 MiB 2.32 MiB 610.45 KiB
b1066e6 1.73 MiB 2.32 MiB 610.45 KiB

@codecov-commenter
Copy link

Codecov Report

Base: 80.36% // Head: 80.35% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (06a4570) compared to base (82f359f).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                                   Coverage Diff                                    @@
##             feat/do-not-attach-empty-trace-and-baggage-headers    #2386      +/-   ##
========================================================================================
- Coverage                                                 80.36%   80.35%   -0.02%     
  Complexity                                                 3695     3695              
========================================================================================
  Files                                                       292      292              
  Lines                                                     13795    13797       +2     
  Branches                                                   1822     1823       +1     
========================================================================================
  Hits                                                      11086    11086              
- Misses                                                     2000     2001       +1     
- Partials                                                    709      710       +1     
Impacted Files Coverage Δ
...va/io/sentry/config/ClasspathPropertiesLoader.java 85.00% <50.00%> (-9.45%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adinauer adinauer changed the title Initialize Sentry in OTEL Java Agend and allow configuring it Initialize Sentry in OTEL Java Agent and allow configuring it Nov 23, 2022
@adinauer adinauer mentioned this pull request Nov 23, 2022
21 tasks
}
}
} catch (Exception e) {
// ignore
Copy link
Member

@romtsn romtsn Nov 25, 2022

Choose a reason for hiding this comment

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

l: can we use logger here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

final @Nullable String otelVersion =
mainAttributes.getValue("Sentry-Opentelemetry-Version-Name");
if (otelVersion != null) {
sdkVersion.addPackage("maven:io.opentelemetry:opentelemetry-sdk", otelVersion);
Copy link
Member

@romtsn romtsn Nov 25, 2022

Choose a reason for hiding this comment

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

m: do we also add 3rd-party version to the sdk context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, discussed with someone from backend team. Should be fine. Is there a better place to put it?

Copy link
Member

Choose a reason for hiding this comment

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

packages/modules maybe? that's where we put the 3rd-party stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean this:

private @Nullable Map<String, String> modules;

That's for errors, not transactions unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

ah yeah, then it's probably fine like that

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Can we add a README to sentry-opentelemetry/* that roughly describes instructions for using the agent? We can rely on that while in the mean time before we have proper docs.

@adinauer adinauer merged commit 41586a4 into feat/do-not-attach-empty-trace-and-baggage-headers Nov 28, 2022
@adinauer adinauer deleted the feat/allow-configuring-sentry-otel-agent branch November 28, 2022 10:23
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

4 participants