Conversation
rollbar/contrib/pyramid/__init__.py
Outdated
| @@ -51,6 +53,7 @@ def rollbar_tween_factory(pyramid_handler, registry): | |||
|
|
|||
| def rollbar_tween(request): | |||
| # for testing out the integration | |||
There was a problem hiding this comment.
Looks like this comment got separated from the try block it references.
rollbar/lib/session.py
Outdated
| break | ||
|
|
||
| if not baggage_header: | ||
| return _build_new_session_attributes() |
There was a problem hiding this comment.
This layer should not try to create a session id if not present in the header.
There was a problem hiding this comment.
Ah yes, when I updated the ticket & dmed you I included that change, apologies for not calling it out more:
If a baggage header is not present, one should be created, and a rollbar.execution.scope.id should be created as a hex representation of a 128 bit number. If rollbar.session.id is not present, the key should not be created.
It'd be the rollbar.execution.scope.id that would be created on the fly, rollbar.session.id can be null
There was a problem hiding this comment.
This has been updated to generate rollbar.execution.scope.id instead of rollbar.session.id. I also updated the logic to always create rollbar.execution.scope.id even if rollbar.session.id exists but rollbar.execution.scope.id does not. This was not the case previously.
rollbar/__init__.py
Outdated
| request = _session_data_from_request(data) | ||
| if request is None: | ||
| return | ||
| set_current_session(request.get('headers', None)) |
There was a problem hiding this comment.
It looks like we get here because no middleware had set a current session, but the request is present via the rollbar payload, so we look at the headers there. That makes sense, though I'm curious whether this is to cover unsupported frameworks, or supported ones when the middleware wasn't installed.
I think calling set_current_session here introduces a bug though. Unlike in the middleware, nothing calls reset_current_session, and on the next request (from a different session) get_current_session above will succeed with the stale session that was set here.
This can be avoided if session.py provides a method to return the session_data from the headers without needing to set and get current session.
There was a problem hiding this comment.
That makes sense. I have updated this, as well as added some logic to prevent overwriting any existing attributes. This is mostly to prevent bugs incase our future selves don't notice we were completely overwriting the data['attributes'] key.
| headers = request.get('headers', {}) | ||
| elif hasattr(request, 'headers'): | ||
| headers = request.headers | ||
| set_current_session(headers) |
There was a problem hiding this comment.
Ideally this could go in middleware.py, allowing reset_current_session to be used, and keeping the responsibility of store_current_request more focused.
This function already returns request, so it seems straightforward to move this logic.
There was a problem hiding this comment.
I have moved it to asgi/middleware.py as well as starlette/middleware.py so that it can apply to all ASGI frameworks that utilize the middleware.
|
@danielmorell nice PR! I'll defer to @shankj3 on approval. |
shankj3
left a comment
There was a problem hiding this comment.
This is looking great, thanks Daniel! If we can address the potential bugs and remove the creation of session ids in favor the the execution scope id, I think we'll be ready to ship.
Will the request interception be in a separate PR?
| @@ -9,6 +9,7 @@ | |||
| from unittest import mock | |||
There was a problem hiding this comment.
One thing I noticed when testing flask, we don't actually hook into the request lifecycle. So for my test route, which looks like this
@app.route("/boom_nested")
def boom_nested():
from nested_error import raise_error
rollbar.report_message("Running a message")
raise_error()If no baggage headers are passed, the execution id for the message here, then the exception thrown from raise-error, are different, even though they are in the same execution context.
What do you think about providing a new function for flask config, that would take in the flask app as an argument then hook into app.before_request and app.teardown_request? Then we could use the same logic as other frameworks, with set_current_session and reset_current_session
There was a problem hiding this comment.
I'm also seeing the same problem as with flask, where if you don't pass baggage headers, the rollbar.execution.scope.id is different for each error within a request. This was my little test app, it might be missing something:
import os
import rollbar
from fastapi import FastAPI
from rollbar.contrib.fastapi import add_to
rollbar.init(
access_token=os.getenv("ROLLBAR_TOKEN", "POST_SERVER_ITEM_ACCESS_TOKEN"),
environment=os.getenv("ROLLBAR_ENV", "development"),
)
app = FastAPI(title="rollbar-fastapi-test")
add_to(app)
@app.get("/boom_nested")
async def boom_nested():
from nested_error import raise_error
rollbar.report_message("Error") # has a different request.execution.scope.id
raise_error() # raises exc that has a different request.execution.scope.id| self.app = create_app() | ||
| init_rollbar(self.app) | ||
| self.client = self.app.test_client() | ||
| reset_current_session() |
There was a problem hiding this comment.
Why do we need to reset current session? Is something leaking between requests?
| return _build_new_scope_attributes() | ||
|
|
||
| # Always ensure we have an execution scope ID, even if the baggage header is present but doesn't contain it. | ||
| if not has_scope_id: |
There was a problem hiding this comment.
One last comment here - we should move this out of the core session code, and into the middleware. The reason for this is that we want a single rollbar.execution.scope.id for the lifespan of a request, and in the future, forwarded to other outgoing requests.
If the originating request does'nt have the rollbar.execution.scope.id, then this will be a unique value for every rollbar message sent as the request is processed, which doesn't give us much of a signal.
If this scope id was built once, in the middleware when a request is received and saved to thread / async storage, then we will get that signal.
Description of the change
Adds support for the
baggagerequest header, enabling propagation ofrollbar.session.idandrollbar.execution.scope.id.Frameworks included are:
The
session_idattribute is persisted across the entire request. Every log message, trace, or exception reported will include the samesession_id.If the
baggagehearder is not included in a request, a newsession_idwill be generated, and will persist for the lifecycle of the request.The
session_idis stored in either thread local or an async safecontextvardepending on the framework.Type of change
Related issues
Checklists
Development
Code review