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

feat(mongo instrumentation): added response hook option #533

Merged
merged 1 commit into from Jul 1, 2021

Conversation

prsnca
Copy link
Contributor

@prsnca prsnca commented Jun 14, 2021

Which problem is this PR solving?

  • Add the ability to collect the response of a mongo action (as an optional configuration). This data can be used for monitoring purposes.

Short description of the changes

  • Added responseHook configuration member which is called at the end of each mongo action

@prsnca prsnca requested a review from a team as a code owner June 14, 2021 10:02
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 14, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #533 (9b4342b) into main (bbf298d) will increase coverage by 0.25%.
The diff coverage is n/a.

❗ Current head 9b4342b differs from pull request most recent head 9b894a2. Consider uploading reports for the commit 9b894a2 to get more accurate results

@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   94.72%   94.98%   +0.25%     
==========================================
  Files         169      162       -7     
  Lines       10638    10163     -475     
  Branches     1062     1004      -58     
==========================================
- Hits        10077     9653     -424     
+ Misses        561      510      -51     
Impacted Files Coverage Δ
...ws-lambda/test/integrations/lambda-handler.test.ts 97.71% <0.00%> (-0.16%) ⬇️
...-instrumentation-aws-lambda/src/instrumentation.ts 91.72% <0.00%> (-0.07%) ⬇️
...opentelemetry-instrumentation-mongodb/src/types.ts 100.00% <0.00%> (ø)
...y-instrumentation-memcached/src/instrumentation.ts
...metry-instrumentation-memcached/test/index.test.ts
...telemetry-instrumentation-memcached/src/version.ts
...y-instrumentation-cassandra/src/instrumentation.ts
...telemetry-instrumentation-cassandra/src/version.ts
...entelemetry-instrumentation-memcached/src/utils.ts
...umentation-cassandra/test/cassandra-driver.test.ts
... and 2 more

@prsnca prsnca force-pushed the add-mongo-response-hook branch 2 times, most recently from 922f96e to fd65df7 Compare June 20, 2021 13:33
@prsnca prsnca force-pushed the add-mongo-response-hook branch 2 times, most recently from 9517cfc to e230086 Compare June 24, 2021 07:18
assert.ifError(err);
const spans = memoryExporter.getFinishedSpans();
const findSpan = spans[0];
const spanResult = JSON.parse(
Copy link
Member

Choose a reason for hiding this comment

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

the name spanResult is not clear IMO. it's the hookAttributeValue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, changed to hookAttributeValue to make it more clear

const spans = memoryExporter.getFinishedSpans();
const findSpan = spans[0];
const spanResult = JSON.parse(
findSpan.attributes[dataAttributeName]?.toString() || '{}'
Copy link
Member

Choose a reason for hiding this comment

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

Current implementation gives the impression that findSpan.attributes[dataAttributeName] can be undefined (by using the ? and setting a default value).
But that's something that the test should assert instead.
If the attribute is missing, we will just fail few lines below when trying to access spanResult.cursor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added null validations inside the assertion - now if the findSpan.attributes[dataAttributeName] is undefined, the assertion will fail.

Copy link
Member

@blumamir blumamir Jun 28, 2021

Choose a reason for hiding this comment

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

cool, but still this code is written in a way that implies that findSpan.attributes[dataAttributeName] can be legitimately falsy, where the purpose of the test is exactly to assert that it is not.
I find it a bit confusing, but that's just a matter of personal style I suppose. If you prefer to have it this way, I'm good with it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as the test above... if its empty - the test will fail.

@prsnca prsnca force-pushed the add-mongo-response-hook branch 2 times, most recently from 0254d13 to d75cf00 Compare June 28, 2021 14:49
@blumamir
Copy link
Member

LGTM (there are still some nonblocking minors that can be optionally addressed).

@prsnca prsnca force-pushed the add-mongo-response-hook branch 2 times, most recently from 9bb6010 to 9257eb5 Compare June 28, 2021 15:30
@dyladan
Copy link
Member

dyladan commented Jun 28, 2021

Conversations which have been addressed and resolved should be marked as resolved please

@blumamir
Copy link
Member

blumamir commented Jun 28, 2021

Conversations which have been addressed and resolved should be marked as resolved please

I don't have permissions to resolve conversations. Marked with emoji what has been addressed from the comments I wrote.

@prsnca prsnca force-pushed the add-mongo-response-hook branch 2 times, most recently from e3669b0 to 89d4a35 Compare June 30, 2021 09:53
@prsnca
Copy link
Contributor Author

prsnca commented Jun 30, 2021

Conversations which have been addressed and resolved should be marked as resolved please

All conversations are resolved now, thank you :)

@blumamir
Copy link
Member

LGTM (there are still some nonblocking minors that can be optionally addressed).

Everything has been addressed. LGTM

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM but this is out of date with the base branch and I don't have permission to update it.

@prsnca
Copy link
Contributor Author

prsnca commented Jun 30, 2021

LGTM but this is out of date with the base branch and I don't have permission to update it.

Rebased over main

@vmarchaud vmarchaud merged commit b9e00e7 into open-telemetry:main Jul 1, 2021
@dyladan dyladan added the enhancement New feature or request label Jul 2, 2021
@nozik nozik deleted the add-mongo-response-hook branch July 6, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants