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

Updates StackExchange.Redis v2 and above to use a new wrapper with improved performance #1351

Conversation

jaffinito
Copy link
Member

Description

There are two improvements in the PR, a general one and a redis specific one.

General

This includes a change to how the local hostname for various datastore segments is determined. Previously, if we detected that the target server is localhost, we would make a call to Dns.GetHostName() to determine the local hostname and this would occur every time that NormalizeHostname is called which is every time a datastore method is hit. This call can take a while to return since it has to reach out to the network. I noticed in my local testing that this was very expensive.

We already have the local hostname for the server, captured at agent startup for utilization data. This PR updates NormalizeHostname to take in a a local hostname and that is used instead. The logic remains the same for otherwise.

This change results in all the datastore segments and any related unit tests needing an small update resulting in the large number of changed files in the PR.

General StackExchange.Redis

In both version 1 and version 2 of StackExchange.Redis there is a profiling API that can be used to capture data on the commands that are being executed, including all the data we want to capture. Unfortunately, in v1, the API requires the user to explicitly start and stop the sessions. Since we don't control this or have visibility to this, we can't use the profiling API for v1.

Starting in v2, the API was moved to a call-back model that is handled by the library which enabled us to use this to capture data.

Old StackExchange.Redis

This updates the v1 instrumentation using assembly versioning to limit it to v1 only, otherwise it leaves it as is.

New StackExchange.Redis

Due to the amount of interaction necessary with StackExchange.Redis types, it is much easier for us to use use the real types so we are referencing the v2 StackExchange.Redis nuget.

SessionCache

As part of the new call-back based profiler, we will need to manage the session lifecycle. This is done in the SessionCache. The SessionCache does a few things:

  • Stores all the ProfilingSession objects in a ConcurrentDictionary using the calling segment spanid as the key.
  • Holds the call-back Func, GetProfilingSession, that either returns an existing ProfilingSession or creates and stores a new one before returning it.
  • Has a Harvest method that is called when segments end, if there is anything in the SessionCache. If the cache is null or empty it just skips.
  • Has a GetConnectionInfo method that captures the datastore instance information from each command. This is a simplified version from the old instrumentation that reduces the number of allocations that occur.
  • The SessionCache is not specific to any ConnectionMultiplexor so only one will exist at anytime.

SessionCache derives from an interface called IStackExchangeRedisCache to keep us from referencing StackExchange.Redis in the agent Core. This method interface has just two methods on it, Harvest and Count.

The SessionCache is stored in the Agent class, in the IAgentExperimental interface and is instantiated by the new instrumentation.

GetProfilingSession
This Func performs some validation to ensure that we only create sessions on valid transactions and segments. It also passes in the Transaction object so that we can get at it later. As noted in the method, during async operations, the transaction can get lost and report as NoOp so we store a reference to it in the session.

Harvest
This is called by and updated Segment.End if there are any sessions in the cache. It returns immediately if it cannot find the spanid in the collection. This collects the commands from the ProfilingSession so that we can create segments from them in a batch. Segment creation works using a new method for starting segments called Transaction.StartStackExchangeRedisSegment that takes in both the start and end time of the new segment as well as new SegmentEndStackExchangeRedis method that ends the segment using the already provided timing data.

Segment
A new ctor was added that takes in relative end time in addition to the usual start time. The End and Finish method were refactored to move the endTime code from Finish to End to account for the endTime already being set. I also moved Finish next to End to readability reasons. The code that calls into SessionCache.Harvest is found in the End method. It is within the endTime if..then to improve efficiency since we don't want Harvest called when creating the redis command segments.

Wrapper

  • Instruments StackExchange.Redis.ConnectionMultiplexer.CreateMultiplexer which is called in all the Connect methods for all v2. When we instrument the object, it has not yet connected to anything so we moved the ConnectionInfo from here into the SessionCache as noted above.
  • This will check for an existing SessionCache and create one if needed. Only the very first time we instrument a ConnectionMultiplexor will we need to create one since the SessionCache is not specific to any ConnectionMultiplexor.
  • Registers the call-back to the instrumented ConnectionMultiplexor and thats really it. All the real work is done in the SessionCache eliminating the need to profile individual DB calls which breaks the ConnectionMultiplexor.

Unbounded Tests

  • Updates the versions of StackExchange.Redis we test for better coverage.
  • Updates the "Async" StackExchange.Redis to use ConnectAsync instead of Connect since using that method changes how the ConnectionMultiplexor is created breaking several variations of the instrumentation I tried.

Other

  • Updated MSI with new instrumentation
  • Updated ArtifactBuilder with new instrumentation

Author Checklist

Reviewer Checklist

  • Perform code review
  • Pull request was adequately tested (new/existing tests, performance tests)
  • Review Changelog

Copy link
Member

@chynesNR chynesNR left a comment

Choose a reason for hiding this comment

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

I think this all looks good. Is there a more central place we could have cached the host name to avoid passing it around everywhere?

@jaffinito
Copy link
Member Author

I wanted to avoid duplicating the existing utilizationHostname in some other location just for this. I didn't want to start adding references to the agent every either so I went with passing it around from places that mostly had it already. Going with the agent ref approach is equally valid if we want to explore that.

@jaffinito jaffinito merged commit c8265f2 into instrumentation-version-filtering-feature Jan 20, 2023
@jaffinito jaffinito deleted the stackexchangeredis-v2plus-updates branch January 20, 2023 22:54
chynesNR added a commit that referenced this pull request Jan 24, 2023
* Instrumentation by assembly version
* Updating precompiled Profiler binaries
* Removed special case System.Net.Http logic (#1344)
* Add support for RestSharp versions between 106.6.10 and 107.x (#1352)
* Updates StackExchange.Redis v2 and above to use a new wrapper with improved performance (#1351)
* Add new StackExchange.Redis instrumentation to MSI
* Add integration test coverage for RestSharp v107+ (#1356)
* Update CHANGELOG.md

Co-authored-by: Alex Hemsath <57361211+nr-ahemsath@users.noreply.github.com>
Co-authored-by: Jacob Affinito <jaffinito@newrelic.com>
Co-authored-by: Josh Coleman <83677148+JcolemanNR@users.noreply.github.com>
Co-authored-by: Josh Coleman <jcoleman@newrelic.com>
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.

None yet

2 participants