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

Ref: Make hints Map<String, Object> instead of only Object #1929

Merged
merged 12 commits into from
Mar 4, 2022

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Mar 1, 2022

📜 Description

Ref: Make hints Map<String, Object> instead of only Object

💡 Motivation and Context

Closes #1558

💚 How did you test it?

Adapted unit tests.

📝 Checklist

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

🔮 Next steps

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2022

Codecov Report

Merging #1929 (ff8d394) into 6.x.x (e948a50) will increase coverage by 0.04%.
The diff coverage is 69.37%.

Impacted file tree graph

@@             Coverage Diff              @@
##              6.x.x    #1929      +/-   ##
============================================
+ Coverage     80.48%   80.52%   +0.04%     
- Complexity     2907     2910       +3     
============================================
  Files           213      214       +1     
  Lines         10776    10851      +75     
  Branches       1427     1432       +5     
============================================
+ Hits           8673     8738      +65     
- Misses         1587     1595       +8     
- Partials        516      518       +2     
Impacted Files Coverage Δ
...vlet/SentryRequestHttpServletRequestProcessor.java 94.73% <ø> (ø)
...ring/SentryRequestHttpServletRequestProcessor.java 100.00% <ø> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
.../sentry/DuplicateEventDetectionEventProcessor.java 100.00% <ø> (ø)
sentry/src/main/java/io/sentry/EventProcessor.java 50.00% <ø> (ø)
sentry/src/main/java/io/sentry/HubAdapter.java 5.00% <ø> (ø)
sentry/src/main/java/io/sentry/IHub.java 86.36% <ø> (ø)
sentry/src/main/java/io/sentry/ISentryClient.java 85.71% <ø> (ø)
...ntry/src/main/java/io/sentry/NoOpSentryClient.java 60.00% <0.00%> (ø)
sentry/src/main/java/io/sentry/Scope.java 97.02% <ø> (ø)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e948a50...ff8d394. Read the comment docs.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

I skimmed through it and the only question I have is: Do we plan on documenting these hint keys so folks can find/use them in beforeSend/eventProcessors?

Perhaps it'd be best to have them available in public static String's and use some sort of prefixing. To avoid conflict as more hints are added and to be easier to understand what the value represents.

@marandaneto
Copy link
Contributor Author

I skimmed through it and the only question I have is: Do we plan on documenting these hint keys so folks can find/use them in beforeSend/eventProcessors?

Perhaps it'd be best to have them available in public static String's and use some sort of prefixing. To avoid conflict as more hints are added and to be easier to understand what the value represents.

Yes, the idea is to document the considered public ones, as JS does, https://docs.sentry.io/platforms/javascript/configuration/filtering/#hints-for-breadcrumbs

Some of them are already documented, e.g. originalException, syntheticException and event_id.
I'd document tho that we don't support originalException and event_id since the Event itself has these properties.

We could make a class with those constants or really just document it via docs.

@romtsn
Copy link
Member

romtsn commented Mar 2, 2022

I think I'd also vote for having a class with constants, then all the keys are in the same place, which is easier for us and for those who is looking at the public API.

Another thing I noticed - some of the keys are capitalized and some are full-lowercase - should we align all of them and use just lowercase? (I guess the lowercase comes from JS)

@ApiStatus.Internal public static final String SENTRY_TYPE_CHECK_HINT = "sentry:typeCheckHint";

/** Used for Synthetic exceptions. */
public static final String SENTRY_SYNTHETIC_EXCEPTION = "syntheticException";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept as it is since its part of the common docs for hints

@marandaneto marandaneto merged commit 736f4a2 into 6.x.x Mar 4, 2022
@marandaneto marandaneto deleted the feat/hints-map branch March 4, 2022 12:51
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.

4 participants