Skip to content

Conversation

randomanderson
Copy link
Contributor

This pull request adds operation_id and network tags to Couchbase requests as specified here. The main implementations differences from that RFC are:

  • I did not implement peer.service because that would alter our current service naming
  • I used the OpenTracing network tags instead of peer.address.

Version 2.6.0 of Couchbase is the first version where operation_id is specified on most requests so that is the base version instrumented.

@randomanderson randomanderson requested a review from a team as a code owner September 16, 2019 21:11
Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

A few requests below...


// 2.6.0 and above
public static void muzzleCheck(final JsonCryptoTranscoder transcoder) {
transcoder.documentType();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed in both instrumentation classes? I usually prefer adding this only if there is misalignment. What is the limiting factor that makes this integration only work with 2.6 instead of 2.5.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The network instrumentation relies on the core instrumentation to add the span to the ContextStore. If the core instrumentation is turned off because the user is running pre-2.6, the network instrumentation will be a noop every time. Might as well turn off the network instrumentation too.


operationId() was added to CouchbaseRequest in 2.6 . Before that, for each message type, there was a different way of extracting the id (including parsing JSON strings). Also, different versions have different message types so it would involve creating an instrumentation for each of couchbase-2.1, couchbase-2.2, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case this only needs to be added here since core advice will get muzzled automatically because of the use of operationId(). (IE, remove the check from the core instrumentation class.

@randomanderson randomanderson force-pushed the landerson/couchbase-op-id branch from 99707a1 to 835450c Compare September 18, 2019 20:07
@randomanderson randomanderson force-pushed the landerson/couchbase-op-id branch from d038f72 to 43cbf7a Compare September 18, 2019 21:15
Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Nice work. Go ahead and merge once that method is removed.

}

// 2.6.0 and above
public static void muzzleCheck(final JsonCryptoTranscoder transcoder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method can be removed.

@randomanderson randomanderson merged commit e6c061f into master Sep 19, 2019
@randomanderson randomanderson deleted the landerson/couchbase-op-id branch September 19, 2019 19:57
@tylerbenson tylerbenson added this to the 0.34.0 milestone Oct 11, 2019
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.

2 participants