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

Major release 9 #1810

Merged
merged 134 commits into from
Feb 14, 2023
Merged

Major release 9 #1810

merged 134 commits into from
Feb 14, 2023

Conversation

hannahramadan
Copy link
Contributor

@hannahramadan hannahramadan commented Feb 8, 2023

Includes all changes for the 9.0 release

All PRs included in this branch

disabling tracing correctly
concurrent_invocation to work with thread tracing
Enable `instrumentation.thread.tracing` by default
It was deprecated, but the agent still uses it for the server side value
but it can no longer be set by users through newrelic.ym or anything
@tannalynn tannalynn marked this pull request as ready for review February 13, 2023 19:45
@tannalynn
Copy link
Contributor

ci cron run
jruby run

tannalynn
tannalynn previously approved these changes Feb 13, 2023
@@ -9,7 +9,7 @@
DependencyDetection.defer do
# Why not just :grape? newrelic-grape used that name already, and while we're
# not shipping yet, overloading the name interferes with the plugin.
named :grape_instrumentation
@name = :grape_instrumentation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did we make this change to select instrumentation and not others?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was related to the configuration options we removed. Several of these are called something_instrumentation, but our configuration options are instrumentation.something. We have tests that expect configuration options called instrumentation.something_instrumentation when named is used, so we had to switch it to just set the name attribute directed.

@github-actions
Copy link
Contributor

SimpleCov Report

Coverage Threshold
Line 93.94% 93%
Branch 85.39% 84%


instrumentation_methods :chain, :prepend

gemfile nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is this envfile doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not much lol. So we need to test the chain an prepend instrumentation, but since this is for the thread and fiber classes, there are no gems we need to install to test this, so we pass nil so it only creates a gemfile with the base multiverse gems we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, that makes sense :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This code (equivalent to gemfile(nil)) is invoking the gemfile method defined in multiverse/lib/multiverse/envfile.rb with nil as the argument.

It looks like doing so would accomplish the same as removing this gemfile nil line entirely. Perhaps it should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously I was operating under the assumption that a gemfile call was needed to get a Gemfile.1 file created. But given this line in the gemfile method:

@gemfiles.push(content) unless content.nil? || content.empty?

that doesn't appear to be the case.

threads = []

threads << ::Thread.new do
# nothong
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

Copy link
Contributor Author

@hannahramadan hannahramadan left a comment

Choose a reason for hiding this comment

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

Looks good!

@tannalynn tannalynn merged commit 9f4d2bb into dev Feb 14, 2023
@hannahramadan hannahramadan deleted the major-release-9 branch March 2, 2023 17:35
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.

4 participants