-
Notifications
You must be signed in to change notification settings - Fork 514
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
feat(measurements): Add experimental set_measurement api on transaction #1359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this better then the plural version. Very clean interface. This way one can set and update values.
|
||
events = capture_events() | ||
|
||
transaction = start_transaction(name="measuring stuff") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I do:
child = transaction.start_child(x)
can I do child.set_measurement(x, y)
too?
Or start_child
would return a different interface that does not provide set_measurement
? since measurements are only available at the transaction level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently they are just on transaction and not span, start_child
returns a span so you can't add measurements there. From what I understood of the requirements, we only want to expose this api on the transaction but this is a design decision and we can also change it.
Open decision is also if we want to expose this on the hub/top-level api as well where it'll just fetch the current hub/scope/transaction and set it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should not leak this API to the Child level as well, but a few SDKs return the very same interface for start_transaction
and start_child
, so unless we change that, the public API will leak.
cc @bruno-garcia
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
3036ecb
to
28662c0
Compare
62a0ac6
to
822ad0f
Compare
tested end-end with relay/ingestion
|
I went with
set_measurement(name, value, unit)
(singular) instead ofset_measurements
as in JS since I think this is more convenient for the end user. Otherwise they need to build the name -> unit/value dict themselves.This should be merged once develop is synced and the changes in relay have been made to support units for
measurements
.