-
Notifications
You must be signed in to change notification settings - Fork 824
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: add redis plugin example #534
feat: add redis plugin example #534
Conversation
Codecov Report
@@ Coverage Diff @@
## master #534 +/- ##
=========================================
- Coverage 90.38% 86.88% -3.5%
=========================================
Files 150 143 -7
Lines 7590 7123 -467
Branches 666 645 -21
=========================================
- Hits 6860 6189 -671
- Misses 730 934 +204
|
- SpanContext Propagation (from Client to Server) | ||
- Span Events | ||
- Span Attributes | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain a little more on actual example: what operations are we executing and what are the components involved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, added few comments
examples/redis/setup.js
Outdated
function setupTracerAndExporters(service) { | ||
const tracer = new NodeTracer({ | ||
plugins: { | ||
http: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, http
plugins is enabled
with correct path
, do you still want to add here? Once #503 merged, will add that into default supported plugins list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I specify redis plugin here, will that "overwrite" the default list and only load the redis plugin and not any other default plugins? Ultimately this is all moot once redis becomes a default plugin, so I'm fine with blocking this until it becomes default (and making this just a default new NodeTracer()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the plugins
object overwrites the default, it does not merge with it. In any case, the redis plugin is now in the default list anyways so this doesn't matter.
|
||
```sh | ||
# from this directory | ||
npm run docker:start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quiestion: if I want to start zipkin and jaeger at the same time, should I run this command twice or can be already omitted. Maybe just add some additional information here ?
|
||
tracer.withSpan(span, async () => { | ||
try { | ||
const res = await axios.get('http://localhost:8080/run_test'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you replace axios with standard http
to avoid 3rd parties ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am currently using axios since it removes a lot of the unimportant "noise code" in the sample. Currently nothing interesting happens in client.js
other than kicking off the trace, so I would prefer to keep axios here so that the sample highlights the redis usage. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was just to have consistency within all the examples and codebase. So far we don't use axios, but rather just standard libraries as then the user can choose what he wants. If that's the way to go then fine I just think we should have consistency :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't mind the use of axios here. In fact, it shows that third party libraries can be used. These are meant to be examples of real world use and almost nobody uses the http
library directly. These aren't shipped to customers in the bundle so additional dependencies don't have as big a burden as they do in the packages
|
||
// Require in rest of modules | ||
const express = require('express'); | ||
const axios = require('axios').default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you replace axios with standard http
to avoid 3rd parties ?
@open-telemetry/javascript-approvers Please review when you get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
examples/redis/package.json
Outdated
"devDependencies": { | ||
"cross-env": "^6.0.0" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 67b46cc
examples/redis/package.json
Outdated
}, | ||
"keywords": [ | ||
"opentelemetry", | ||
"http", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"http", | |
"redis", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 67b46cc
Which problem is this PR solving?
Short description of the changes