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

support for logback encoders has disappeared? #2217

Closed
jeacott1 opened this issue Aug 12, 2022 · 8 comments
Closed

support for logback encoders has disappeared? #2217

jeacott1 opened this issue Aug 12, 2022 · 8 comments

Comments

@jeacott1
Copy link

Problem Statement

A feature existed but appears to have been removed - why?
ref: #794
this PR introduced Encoder support for the sentry Logback appender, but the current code appears to have no trace of this merge.
what happened to it?

Solution Brainstorm

No response

@adinauer
Copy link
Member

Hello @jeacott1, there was a rewrite of the SDK in 2020. Seems like encoder support for logback got lost in the process. At least I couldn't find any deliberate decision to remove / not include it.

I assume you'd like to have it back in there. We can add it to the backlog but unfortunately I can't tell you a timeline for it.

@jeacott1
Copy link
Author

@adinauer yes please. the logback appender is a very convenient way to capture all the things without worrying about the otherwise very thread centric nature of sentry. And the encoder made it possible to inject global, dynamic content.
I'm surprised to see MDC dumped entirely actually. Its also a nice, relatively implementation agnostic way to inject custom content for sentry, both global and per each.

@adinauer
Copy link
Member

@jeacott1 MDC entries should land in the event context (see https://docs.sentry.io/platforms/java/guides/logback/usage/advanced-usage/). You may also convert them to tags (see

<appender name="sentry" class="io.sentry.logback.SentryAppender">
<options>
<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>
<contextTag>userId</contextTag>
<contextTag>requestId</contextTag>
</options>
<!-- Demonstrates how to modify the minimum values -->
<!-- Default for Events is ERROR -->
<minimumEventLevel>WARN</minimumEventLevel>
<!-- Default for Breadcrumbs is INFO -->
<minimumBreadcrumbLevel>DEBUG</minimumBreadcrumbLevel>
</appender>
).

@jeacott1
Copy link
Author

@adinauer thanks - I thought I'd seen that mdc was removed, glad to see that survives.
Most of the time I want to add global extras though.
I can somewhat do it with the Sentry init options, but that is heavily thread centric and sometimes I want sentry to init immediately but need to add details asap but not yet knowable, to support the remainder of the execution (think spring boot where I want to capture the startup before spring has been initialised and I cant yet access details I want for the bulk of the execution).
with the encoder in place all I needed to do was add a Logback logContext.putProperty("global_tag", "something I care about"); once, and I can do that at anytime, and it would always work. I didn't need to scatter Sentry calls throughout nor repeatedly add MDC for every log.

I think there is a case for restoring the encoder option.

@jeacott1
Copy link
Author

jeacott1 commented Aug 16, 2022

@adinauer on a related issue that I don't seem to be getting my head around.
For the case where I am using the logback appender, and I want to have the server show me the IP address for each event (from the server's perspective) - I think this is equivalent to setting user ip {{auto}} true? - but I don't want to set Pii true and capture other content - I only want the already public IP address. - is this only available in a self hosted sentry? is there some way I can enable this? I've tried the docs, but I just don't seem to be finding how this should work?

@adinauer
Copy link
Member

@jeacott1 you should be able to have the IP set by the server by explicitly setting the user IP to {{auto}}. See https://docs.sentry.io/platforms/java/enriching-events/identify-user/#ip_address

Does that not work for you?

@adinauer adinauer moved this from Needs Discussion to Backlog in Mobile & Cross Platform SDK Aug 18, 2022
@jeacott1
Copy link
Author

@adinauer if I do that, wont I also get other user related data? - also, given that this is a pure serverside attribute, why cant I choose to see this information regardless of the client config?

@adinauer
Copy link
Member

Closing as it has been implemented again in #2246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants