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(plugin): implement redis plugin #503

Open
wants to merge 15 commits into
base: master
from

Conversation

@markwolff
Copy link
Member

markwolff commented Nov 7, 2019

Which problem is this PR solving?

Short description of the changes

  • Implements Redis plugin to create/propagate spans on client commands.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #503 into master will decrease coverage by 9.71%.
The diff coverage is 87.03%.

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   96.46%   86.75%   -9.72%     
==========================================
  Files         128      140      +12     
  Lines        6455     7015     +560     
  Branches      556      638      +82     
==========================================
- Hits         6227     6086     -141     
- Misses        228      929     +701
Impacted Files Coverage Δ
.../opentelemetry-plugin-redis/test/assertionUtils.ts 100% <100%> (ø)
packages/opentelemetry-plugin-redis/src/redis.ts 100% <100%> (ø)
packages/opentelemetry-plugin-redis/src/enums.ts 100% <100%> (ø)
...kages/opentelemetry-plugin-redis/test/testUtils.ts 15% <15%> (ø)
packages/opentelemetry-plugin-redis/src/utils.ts 91.3% <91.3%> (ø)
...ages/opentelemetry-plugin-redis/test/redis.test.ts 92.85% <92.85%> (ø)
...entelemetry-core/test/common/ConsoleLogger.test.ts 0% <0%> (-100%) ⬇️
...try-core/test/trace/fixtures/test-package/index.js 0% <0%> (-100%) ⬇️
...ges/opentelemetry-core/test/trace/NoopSpan.test.ts 0% <0%> (-100%) ⬇️
...s/opentelemetry-core/test/context/B3Format.test.ts 0% <0%> (-100%) ⬇️
... and 60 more
Copy link
Member

vmarchaud left a comment

Overall good, just little things to have the same config as other plugin

markwolff added 4 commits Nov 8, 2019
Copy link
Member

vmarchaud left a comment

LGTM

@markwolff markwolff marked this pull request as ready for review Nov 11, 2019
Copy link
Contributor

mayurkale22 left a comment

Overall LGTM, added a few minor comments.

@@ -8,11 +8,14 @@
"repository": "open-telemetry/opentelemetry-js",
"scripts": {
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts'",

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Nov 11, 2019

Contributor

Do you think we should remove "private": true to make this package available for the next release?

This comment has been minimized.

Copy link
@markwolff

markwolff Nov 12, 2019

Author Member

Agreed. I removed it from this plugin. I think we should also remove it from other "ready" plugins (separate PR). Seems to just be postgres and http2 currently


export enum AttributeNames {
// required by https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md#databases-client-calls
COMPONENT = 'component',

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Nov 11, 2019

Contributor

I think we should move these common attributes to either core or base package, this is being used in almost all the plugins. WDYT?

This comment has been minimized.

Copy link
@markwolff

markwolff Nov 12, 2019

Author Member

SGTM. I'll create an issue for it.


export class RedisPlugin extends BasePlugin<typeof redisTypes> {
static readonly COMPONENT = 'redis';
readonly supportedVersions = ['^2.6.0']; // should be >= 2.6.0

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Nov 11, 2019

Contributor

Could you please include supported version in README.md?

This comment has been minimized.

Copy link
@markwolff

markwolff Nov 12, 2019

Author Member

Done in 391326b

// New versions of redis (2.4+) use a single options object
// instead of named arguments
if (arguments.length === 1 && typeof cmd === 'object') {
const span = plugin._tracer.startSpan(`redis-${cmd.command}`, {

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Nov 11, 2019

Contributor
Suggested change
const span = plugin._tracer.startSpan(`redis-${cmd.command}`, {
const span = plugin._tracer.startSpan(`${RedisPlugin.COMPONENT}-${cmd.command}`, {
@mayurkale22

This comment has been minimized.

Copy link
Contributor

mayurkale22 commented Nov 11, 2019

It would be awesome if you can include the example (may be in separate PR)?

markwolff added 2 commits Nov 12, 2019
Copy link
Contributor

draffensperger left a comment

Minor style suggestions, but LGTM

private _getPatchInternalSendCommand() {
const plugin = this;
return function internal_send_command(original: Function) {
return function internal_send_command_trace(

This comment has been minimized.

Copy link
@draffensperger

draffensperger Nov 13, 2019

Contributor

Would it make sense to move this function out of the class and pass in plugin as an argument (would need to wrap with an arrow function)? That could reducer the indentation of it

This comment has been minimized.

Copy link
@markwolff

markwolff Nov 14, 2019

Author Member

Is this what you had in mind? 8654a71

}

let spanEnded = false;
const endSpan = (err?: Error | null) => {

This comment has been minimized.

Copy link
@draffensperger

draffensperger Nov 13, 2019

Contributor

Could this be extracted as a helper function at the top level of the module?

This comment has been minimized.

Copy link
@markwolff

markwolff Nov 14, 2019

Author Member

Agreed. I moved it out. I had to get rid of the spanEnded check, but if the span is being ended more than once, its probably a bug that we should log out anyways.

@mayurkale22

This comment has been minimized.

Copy link
Contributor

mayurkale22 commented Nov 14, 2019

@markwolff Could you please resolve Dave's comment and rebase with the master, looks good to go.

@markwolff markwolff requested a review from obecny as a code owner Nov 14, 2019
markwolff and others added 4 commits Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.