-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RFC WIP] Add a new package to enable open tracing #3389
Conversation
efb93f0
to
2e7f706
Compare
c5ae2a7
to
bbb6693
Compare
bdd2d68
to
cc1c062
Compare
ebd2ca8
to
16eae9f
Compare
82c552e
to
602f7ed
Compare
npm install --save @loopback/extension-tracing | ||
``` | ||
|
||
## Basic use |
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.
Very cool and complicated stuff.
The Basic Use
section should provide a description of what default options this component is using, and how it will affect the tracing. Perhaps an example of tracing one request and the output that appears on console or in a log.
Opentracing provides this introductory material, maybe we should link to that page from our tracing extension's README? https://opentracing.io/docs/overview/ There is also a guide aimed specifically at framework developers: https://opentracing.io/docs/best-practices/instrumenting-frameworks/ I need to read those documents first to build a better understanding of the problem domain, before I can provide any meaningful feedback for this pull request. |
const spanStr = span.context().toString(); | ||
await request | ||
.get('/ping') | ||
.set(LOOPBACK_TRACE_ID, spanStr) |
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.
This is not in line with OpenTracing guidelines, see e.g. https://opentracing.io/docs/overview/inject-extract/ and https://opentracing-javascript.surge.sh/classes/tracer.html#inject.
Before we dive into implementation details, I'd like to clarify and reach agreement about the intended user/developer experience and our high-level plan for achieving it. As I was reading through OpenTracing Overview (including sub-sections), The OpenTracing Semantic Specification and Semantic Conventions and [Best practices for instrumenting frameworks](Instrumenting frameworks), a lot of questions come to my mind.
Now I am not saying we need to answer and implement all of the points listed above. What I am looking for is a high-level overview of our master plan for distributed tracing and then an explanation of where this initial pull request fits into that plan, what is its scope and what's intentionally left for later. |
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.
☝️
any news / progress on this feature ? |
I've started similar progress to what is here using opentelemetry-js (as it seems to be the current Cloud Native client library).
I would suggest that it gets backed into the REST component as the extension points are not called early enough. I've noticed that the spans that are created by the library do not encompass the express middleware and http calls.
Since opentelemetry-js wraps the |
Nice. I have been waiting for the opentelemetry to come out.
We can simply change the interceptor to be a middleware. It's very straightforward.
That's cool as we don't have to mess around individual connectors. @lswith Are you willing to contribute a patch for proposed enhancements? I have too many things on my plate at the moment. |
@raymondfeng I'm happy to take over. I'm going to wait for this issue: open-telemetry/opentelemetry-js#752 to resolve though, as it is required before we can use spans across |
Signed-off-by: Raymond Feng <[email protected]>
This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity. |
This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one. |
Any Update on this? Or we Would love to pick it up as we need this for our internal Project. @lswith @raymondfeng |
I have just published loopback4-tracing, you might wanna take a look at it. Few notes about the package
With some additional testing and a few minor updates this should be easy to make production ready again. I am definitely open to contributions to the package, you can fork it, or even just get some inspiration from different parts. Anyways, hope this helps one way or the other to improve loopback <> tracing support. |
@nflaig Hey! thanks for sharing, I am looking into, i'll dive in and push any PR, if needed. |
See
The test depends on #3388(merged).Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈