Skip to content

Pass MDC tags as Sentry tags - #1954

Merged
adinauer merged 9 commits into
6.x.xfrom
gh-1148
Mar 28, 2022
Merged

Pass MDC tags as Sentry tags #1954
adinauer merged 9 commits into
6.x.xfrom
gh-1148

Conversation

@maciejwalkowiak

Copy link
Copy Markdown
Contributor

📜 Description

Add option to specify MDC tag names that are mean to be applied as Sentry tags to Sentry events instead of putting into event contexts as we used to do.

💡 Motivation and Context

Fixes #1148

💚 How did you test it?

Unit & Integration tests.

📝 Checklist

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

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (6.x.x@f710288). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             6.x.x    #1954   +/-   ##
========================================
  Coverage         ?   80.61%           
  Complexity       ?     2946           
========================================
  Files            ?      214           
  Lines            ?    10928           
  Branches         ?     1451           
========================================
  Hits             ?     8810           
  Misses           ?     1592           
  Partials         ?      526           

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 f710288...970e591. Read the comment docs.

@bruno-garcia bruno-garcia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm. @adinauer ?

if (mdcProperties.containsKey(mdcTag)) {
event.setTag(mdcTag, mdcProperties.get(mdcTag));
// remove from all tags applied to logging event
mdcProperties.remove(mdcTag);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bruno-garcia do we want to add entries only to either tags or context not both?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just in tags is fine, no need to dupe 👍

minimumBreadcrumbLevel="DEBUG"
minimumEventLevel="WARN"
debug="true"
mdcTags="userId"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this sample also show how to set multiple?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sample updated.

<debug>true</debug>
<!-- NOTE: Replace the test DSN below with YOUR OWN DSN to see the events from this app in your Sentry project/dashboard -->
<dsn>https://502f25099c204a2fbf4cb16edc5975d1@o447951.ingest.sentry.io/5428563</dsn>
<mdcTag>userId</mdcTag>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this sample also show how to set multiple?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is <mdcTag> correct here? or should it be <mdcTags>?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is correct. under the hood it calls addMdcTag

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sample updated.

private final @NotNull List<String> inAppExcludes = new CopyOnWriteArrayList<>();
private final @NotNull List<String> inAppIncludes = new CopyOnWriteArrayList<>();
private final @NotNull List<String> tracingOrigins = new CopyOnWriteArrayList<>();
private final @NotNull List<String> mdcTags = new CopyOnWriteArrayList<>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can anyone think of a name that's a bit more generic than MDC Tags?
In case any other SDK wants to add support for a similar feature it might be nice if they can use the same name.
Something like logContextTags or promoteLogContextEntriesToTags?

We should then probably mention somewhere that this takes MDC entries and puts them into sentry tags so people can actually find this feature when searching for MDC.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps contextTags? I am open to change it to anything more suitable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

contextTags sounds good to me. Can you please rename it? Sorry for the inconvenience but I'd rather change it now than deprecate and rename later as we've already been asked to pick a name that can be used for more than just the Java SDK.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bruno-garcia bruno-garcia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@adinauer
adinauer merged commit 8bd08ba into 6.x.x Mar 28, 2022
@adinauer
adinauer deleted the gh-1148 branch March 28, 2022 15:27
@verglor

verglor commented Jun 20, 2022

Copy link
Copy Markdown

Hi @adinauer, has this PR been actually released ? It's missing in CHANGELOG.md and documentation.

@adinauer

Copy link
Copy Markdown
Member

Hello @verglor, yes it has been released with version 6.0.0. Seems like it's vanished from the changelog. Docs PR hasn't been merged yet, I'll check if we can merge.

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.

5 participants