Skip to content

Add the stack option back to the Logging integration #176

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

Merged
merged 2 commits into from
Nov 27, 2018

Conversation

jrideout
Copy link
Contributor

In Raven, you could do this:

logger.warning('things happened', extra={'stack': True})

And the stack would be collected. This adds back that feature.

@codecov-io
Copy link

codecov-io commented Nov 21, 2018

Codecov Report

Merging #176 into master will decrease coverage by 0.37%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   80.89%   80.51%   -0.38%     
==========================================
  Files          29       29              
  Lines        2392     2397       +5     
  Branches      402      403       +1     
==========================================
- Hits         1935     1930       -5     
- Misses        302      312      +10     
  Partials      155      155
Impacted Files Coverage Δ
sentry_sdk/integrations/logging.py 88.09% <100%> (+0.75%) ⬆️
sentry_sdk/integrations/aws_lambda.py 0% <0%> (-17.95%) ⬇️
sentry_sdk/utils.py 80.4% <0%> (+0.67%) ⬆️
sentry_sdk/debug.py 96.42% <0%> (+3.57%) ⬆️

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 c1e0ba0...5be012e. Read the comment docs.

@untitaker
Copy link
Member

@jrideout thanks! I believe we want to somehow expose this on the scope instead of having special options on the logger though. That approach did not scale well in Raven.

@jrideout
Copy link
Contributor Author

@untitaker Can you please describe what you mean by scope in this context?

For some context, we've configured our lambda based apps to use:

sentry_logging = LoggingIntegration(level=logging.INFO, event_level=logging.WARNING)
sentry_sdk.init(
    integrations=[sentry_logging, AwsLambdaIntegration()],
    attach_stacktrace=True,
    environment=os.environ.get('SENTRY_ENVIRONMENT', 'dev'),
)

Which is probably sending way too much. I was hoping to turn off attach_stacktrace and use the extra based mechanism for getting stacks when logging.

@untitaker
Copy link
Member

@jrideout we have these scopes to override default values:

with push_scope() as scope:
    scope.fingerprint = ['foobar']

    capture_exception()  # custom fingerprint

capture_exception()  # no custom fingerprint

I wonder whether we can use scopes here too:

with push_scope() as scope:
    scope.attach_stacktrace = True

    logger.error(...)


I realize this is not optimal but I want to get rid of the weird arguments on logging calls

@jrideout
Copy link
Contributor Author

What about using exc_info=True for the non-exception use case as well? That would give a consistent user experience.

@untitaker
Copy link
Member

I like that idea, not sure if we have this info. @mitsuhiko what do you think?

@jrideout
Copy link
Contributor Author

@untitaker looks like we do have the full record in the handler. The logging system converts exc_info to a 3-value tuple of (type, value, traceback). In the case where we aren't in a exception, this tuple is (None, None, None).

@untitaker
Copy link
Member

Ah yes, the (None, None, None) tuple. That should work.

@untitaker
Copy link
Member

@jrideout thanks for your work. Please allow me some time to think it through. I think we're good though

@jrideout jrideout closed this Nov 26, 2018
@untitaker untitaker reopened this Nov 26, 2018
@untitaker
Copy link
Member

Let's keep this open, I will check it out tomorrow

@jrideout
Copy link
Contributor Author

oops, I don't know how I closed this, I didn't mean too!

@untitaker untitaker merged commit cc81d01 into getsentry:master Nov 27, 2018
@untitaker
Copy link
Member

Thanks!

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.

3 participants