Skip to content

Conversation

@cleptric
Copy link
Member

@cleptric cleptric commented Aug 9, 2023

Comment on lines 318 to 319
'request_body_size' => $event->request->toPsrRequest()->getBody()->getSize(),
'http.request.body.size' => $event->request->toPsrRequest()->getBody()->getSize(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a response recieved we do not duplicate them but removed the old names. Should we here too? Also they can be null should we then not include them at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was an oversight, removed!

Hmm, good question, @AbhiPrasad ? 😬

@mfb
Copy link

mfb commented Aug 10, 2023

Are there also standards for how http.server transactions are tagged? I noticed that laravel and symfony do it a bit differently.

@AbhiPrasad
Copy link
Member

Are there also standards for how http.server transactions are tagged

We have some, but they are sparse. If you create a GH issue with the inconsistencies we can take a look and audit.

@mfb
Copy link

mfb commented Aug 10, 2023

We have some, but they are sparse. If you create a GH issue with the inconsistencies we can take a look and audit.

Not sure where to open an issue, but in

url and method are set as data, while in https://github.com/getsentry/sentry-symfony/blob/3e9e23da8985818872cf91e5d5d59895a1421154/src/EventListener/TracingRequestListener.php#L89 http.url and http.method are set as tags.

@cleptric
Copy link
Member Author

Moving forward, we'll remove tags set by the SDK and rely on request/response context and span data instead.

@cleptric cleptric marked this pull request as ready for review August 28, 2023 17:06
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

:shipit:

@cleptric cleptric merged commit c3d2317 into master Aug 29, 2023
@cleptric cleptric deleted the span-attributes branch August 29, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Starfish] Add db attributes to database span's span data

5 participants