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

Adds request data to transaction first, to allow downstream changes #5410

Closed

Conversation

chriswiggins
Copy link

We are using the express handler in our app, but I was puzzled as to why setting the name of the transaction downstream wasn't being respected. Turns out addRequestDataToTransaction was running just before the transaction was sent.

This changes the behaviour to add the data after the transaction is started

@chriswiggins
Copy link
Author

I realise this is not passing tests, but I'm also unsure as to the logic flow and to why this is happening. Would be great to be pointed in the right direction 😄

@Lms24
Copy link
Member

Lms24 commented Jul 27, 2022

Hi @chriswiggins and thanks for your contribution! Apologies for only responding now!

You're right, addRequestDataToTransaction is only called before the transaction is finished. However, we initially set the transaction name right at the start of the transaction. Since you were mentioning the transaction name specifically, I don't think that calling addRequestDataToTransaction earlier would change much here.

However, I want to stress that at the beginning, when we start the transaction, we do not yet have the parameterized route (e.g. users/:id/details) which we want to set as a transaction name by default. Instead, we fall back to the raw URL (e.g. users/123/details). Due to how express does URL matching (and the handling of partial routes), we can only obtain this piece by piece. We recently made improvements here in #5450 where we discuss what's happening (and what we're constrained by) in greater detail.

We're releasing this change today. Please feel free to give it a try and report back. Closing this for the moment but let me know if you would like to see additional changes or if you have ideas how to get fully parameterized routes earlier.

@Lms24 Lms24 closed this Jul 27, 2022
@chriswiggins chriswiggins deleted the tracing-express-handler branch July 27, 2022 19:37
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.

2 participants