-
Notifications
You must be signed in to change notification settings - Fork 600
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
ViewComponent instrumentation #2367
Conversation
7d9162b
to
543cd94
Compare
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.
This looks great overall. 🖼️ 🎉
I know you're still working on things, but don't forget to add a CHANGELOG entry!
I have a few other comments below 🙂
lib/new_relic/agent/instrumentation/view_component/instrumentation.rb
Outdated
Show resolved
Hide resolved
lib/new_relic/agent/instrumentation/view_component/instrumentation.rb
Outdated
Show resolved
Hide resolved
def test_metric_recorded | ||
get('/view_components') | ||
|
||
assert_metrics_recorded('View/view_component/view_component_instrumentation_test.rb/ExampleComponent') |
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.
The metric name looked odd to me with the .rb file path, but it does match the convention we've long had in place for View/
based metrics:
'View/model/index.html.erb/Rendering'
'View/model/_list.html.erb/Partial'
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.
Agree! I don't love the look and thought it was a bit weird, but matches 🤷♀️ It is also how ViewComponent itself has chosen to do their instrumentation (self.class.identifier
is the full path including .rb
)
module ViewComponent | ||
INSTRUMENTATION_NAME = NewRelic::Agent.base_name(name) | ||
|
||
def render_in_with_tracing(*args) |
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'm a bit confused, i'm probably missing something obvious (sorry!).
But this method doesn't seem to take a block or yield or anything, so i'm curious what the segment thats being created is measuring?
begin | ||
segment = NewRelic::Agent::Tracer.start_segment( | ||
name: metric_name(self.class.identifier, self.class.name) | ||
) |
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.
Normally I would expect a yield after this line, which would call super/render_in_without_tracing
from the do blocks.
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.
Great catch @tannalynn. This wasn't yielding and preventing views from showing up 😱 Added the yield, tested, and we get both the NR metric and app view now.
Inline disable rubocop Cleanup tests Appease rubocop
Co-authored-by: Kayla Reopelle (she/her) <[email protected]> Code feedback Add mistakenly deleted file Move file Test update Test updates Use newer vc version
version change Only test latest version
Version fix
5c64014
to
a0e188c
Compare
SimpleCov Report
|
) | ||
yield | ||
rescue => e | ||
::NewRelic::Agent.logger.debug('Error capturing ViewComponent segment', e) |
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.
Late to the party, but... is suppressing errors here the intended behavior?
Add New Relic instrumentation for ViewComponent.
Closes #2205