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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jul 1, 2021
Merged

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

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 June 14, 2021 10:02
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 14, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@prsnca prsnca force-pushed the add-mongo-response-hook branch from b6ed8de to 8a97ea9 Compare June 14, 2021 10:24
@codecov
Copy link

codecov bot commented Jun 20, 2021 •
edited
Loading

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
blumamir
blumamir reviewed Jun 27, 2021
View reviewed changes
plugins/node/opentelemetry-instrumentation-mongodb/src/types.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/src/types.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb.test.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb.test.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb.test.ts Outdated
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

plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb.test.ts Outdated
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 •
edited
Loading

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 from e230086 to 297fe17 Compare June 27, 2021 15:21
@prsnca prsnca force-pushed the add-mongo-response-hook branch 2 times, most recently from 0254d13 to d75cf00 Compare June 28, 2021 14:49
@prsnca prsnca force-pushed the add-mongo-response-hook branch from d75cf00 to b235838 Compare June 28, 2021 15:17
@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 •
edited
Loading

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

dyladan
dyladan approved these changes Jun 30, 2021
View reviewed changes
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 prsnca force-pushed the add-mongo-response-hook branch from 89d4a35 to 99a2879 Compare June 30, 2021 15:31
@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

@prsnca prsnca force-pushed the add-mongo-response-hook branch from 99a2879 to 9b894a2 Compare July 1, 2021 07:48
@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
This was referenced Jul 7, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blumamir blumamir blumamir approved these changes

@MSNev MSNev MSNev approved these changes

@artugam artugam artugam approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@dyladan dyladan dyladan approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@prsnca @blumamir @dyladan @vmarchaud @artugam @MSNev