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

Support Transaction#set_measurement #1838

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Support Transaction#set_measurement #1838

merged 2 commits into from
Feb 3, 2023

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jul 2, 2022

This PR adds the concept of custom measurement to Transaction and a new Transaction#set_measurement API. It should be the Ruby equivalent of getsentry/sentry-python#1359.

Changes

  • Add config.custom_measurements for toggling custom measurement's activation (default: inactive).
  • Add Transaction#measurements and Transaction#set_measurement(name, value, unit = "").

@st0012 st0012 self-assigned this Jul 2, 2022
@st0012 st0012 added this to the 5.4.0 milestone Jul 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2022

Codecov Report

Base: 98.63% // Head: 98.62% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (7b5e21b) compared to base (193446b).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 7b5e21b differs from pull request most recent head bbce93f. Consider uploading reports for the commit bbce93f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1838      +/-   ##
==========================================
- Coverage   98.63%   98.62%   -0.01%     
==========================================
  Files         157      157              
  Lines       10009    10059      +50     
==========================================
+ Hits         9872     9921      +49     
- Misses        137      138       +1     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/configuration.rb 98.51% <100.00%> (+<0.01%) ⬆️
sentry-ruby/lib/sentry/event.rb 98.78% <100.00%> (+0.01%) ⬆️
sentry-ruby/lib/sentry/transaction.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transaction_event.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/configuration_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/transaction_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/breadcrumb.rb 96.29% <0.00%> (-3.71%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator Author

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

@sl0thentr0py Does there any UI for custom measurements? I can see the measurements sent in this transaction, but I can't see anything on the page. I thought it'd be displayed like the web vitals but maybe not?

sentry-ruby/lib/sentry/transaction.rb Show resolved Hide resolved
@sl0thentr0py
Copy link
Member

@st0012 The UI is still WIP. So far only JS and python SDKs have this API, and ingest is partially ready. UI work will follow in the coming month.

@st0012 st0012 force-pushed the add-set_measurement branch from dbd53f2 to 2945569 Compare July 10, 2022 11:04
@st0012
Copy link
Collaborator Author

st0012 commented Jul 10, 2022

@sl0thentr0py I feel it'll be more useful to have something like

transaction.measure("metrics.foo", "millisecond") do
  # operation
end

# and also

Sentry.measure_with_transaction("metrics.foo", "millisecond", transaction: optional_transaction) do
  # operation
end

And then the measurement can be set without manually retrieving the transaction.

@st0012 st0012 requested a review from sl0thentr0py July 10, 2022 11:45
@st0012
Copy link
Collaborator Author

st0012 commented Jul 12, 2022

ping @sl0thentr0py

@sl0thentr0py
Copy link
Member

@st0012 I'll take a look at this tomorrow, but just wanted to say we shouldn't actually ship this till measurements are ready end-end in the product. We added the python/JS stuff to get started internally for development.

@st0012
Copy link
Collaborator Author

st0012 commented Jul 12, 2022

Ah I see. In that case, no need to rush it then 👍

@st0012 st0012 removed this from the 5.4.0 milestone Jul 13, 2022
@github-actions
Copy link

github-actions bot commented Aug 4, 2022

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 Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@sl0thentr0py
Copy link
Member

@st0012 will review this shortly, but if we wanna include this now, I think we don't need this to be experimental anymore.

Re: the top-level API helpers like Sentry.measure_with_transaction, we have to decide internally on naming/behavior but we can ship those separately afterwards.

@st0012
Copy link
Collaborator Author

st0012 commented Sep 30, 2022

If the name is still not settled, I'm fine with merging it later. I was justing seeing if things can be shipped together, not that I wanted to push this 🙂

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Oct 3, 2022

alright, yea let's leave this for the next minor then, the UI is still WIP so it's fine to push this a bit later.

@sl0thentr0py
Copy link
Member

ok UI landed now
getsentry/sentry#39543

@ymatusevich
Copy link

ymatusevich commented Jan 23, 2023

any updates on this? Our team would appreciate this functionality a lot

@st0012
Copy link
Collaborator Author

st0012 commented Feb 1, 2023

@sl0thentr0py So once I rebased this PR, it'll be ok to merge it?

@sl0thentr0py
Copy link
Member

yes I was planning to merge this this week, go ahead!

@st0012 st0012 added this to the 5.8.0 milestone Feb 1, 2023
@st0012 st0012 force-pushed the add-set_measurement branch 2 times, most recently from 838bf54 to 797a51b Compare February 1, 2023 11:49
CHANGELOG.md Outdated
@@ -178,6 +178,22 @@
- Rescue `ThreadError` in `SessionFlusher` and stop creating threads if flusher is killed [#1851](https://github.com/getsentry/sentry-ruby/issues/1851)
- Fixes [#1848](https://github.com/getsentry/sentry-ruby/issues/1848)

- Support `Sentry::Transaction#set_measurement` [#1838](https://github.com/getsentry/sentry-ruby/pull/1838)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This warning is helpful.

CHANGELOG.md Outdated Show resolved Hide resolved
@st0012 st0012 force-pushed the add-set_measurement branch from 797a51b to 7b5e21b Compare February 2, 2023 22:43
CHANGELOG.md Outdated Show resolved Hide resolved
@st0012 st0012 force-pushed the add-set_measurement branch from 7b5e21b to bbce93f Compare February 3, 2023 10:36
@st0012 st0012 requested a review from sl0thentr0py February 3, 2023 10:37
@st0012 st0012 merged commit fcfb53e into master Feb 3, 2023
@st0012 st0012 deleted the add-set_measurement branch February 3, 2023 21:29
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.

4 participants