You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/12/04 17:24:47 UTC
[GitHub] [skywalking-nodejs] tom-pytel opened a new pull request #9: [WIP] express plugin, make spans actually work
tom-pytel opened a new pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9
Would be good to have a chat, this agent was in very poor shape and I could use some clarification to make it work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739439557
> Yes I can help on this, when you think it's all set except for the test, ping me I'll push to this branch directly to verify the plugin (or you'd like to merge this first, just let me know). Thanks :)
Express and https should be good to go so do in whatever order you thinks makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739088004
> I check GitHub notifications most frequently, and reply as soon as I saw them, but due to the different time zones, I might wake up to check them in hours after your posts.
Ok, I will just keep posting here then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740129916
This interacts with http plugin below it just like everything else, and event though they behave well together I am not completely happy with how this works right now. I think it can almost certainly be made more efficient and the interactions better defined (needs testing).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on a change in pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#discussion_r536926836
##########
File path: src/trace/context/SpanContext.ts
##########
@@ -57,39 +58,62 @@ export default class SpanContext implements Context {
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- const span = new EntrySpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- operation,
- });
+ let span;
- if (carrier && carrier.isValid()) {
- span.inject(carrier);
+ if (parent && parent.type === SpanType.ENTRY) {
+ span = parent;
+ parent.operation = operation;
+
+ } else {
+ span = new EntrySpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ operation,
+ });
+
+ if (carrier && carrier.isValid()) {
+ span.extract(carrier);
+ }
}
return span;
}
- newExitSpan(operation: string, peer: string): Span {
+ newExitSpan(operation: string, peer: string, carrier?: ContextCarrier): Span {
if (logger.isDebugEnabled()) {
logger.debug('Creating exit span', {
parentId: this.parentId,
executionAsyncId: executionAsyncId(),
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- return new ExitSpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- peer,
- operation,
- });
+ let span;
+
+ if (parent && parent.type === SpanType.EXIT) {
+ span = parent;
+
+ } else {
+ span = new ExitSpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ peer,
+ operation,
+ });
+
+ // if (carrier && carrier.isValid()) { // is this right?
+ // Object.assign(carrier, span.inject());
Review comment:
I was going off the Python agent, it does something like this:
```python
def new_exit_span(self, op: str, peer: str, carrier: 'Carrier' = None) -> Span:
span = self.ignore_check(op, Kind.Exit)
if span is not None:
return span
spans = _spans_dup()
parent = spans[-1] if spans else None # type: Span
span = parent if parent is not None and parent.kind.is_exit else ExitSpan(
context=self,
sid=self._sid.next(),
pid=parent.sid if parent else -1,
op=op,
peer=peer,
)
if carrier is not None:
span.inject(carrier=carrier)
return span
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 edited a comment on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739439171
> As for tests, I think it is something that would maybe take you 5 minutes but could take me hours to figure out your setup, any chance you could do the test for this?
Yes I can help on this, when you think it's all set except for the test, ping me I'll push to this branch directly to verify the plugin (or you'd like to merge this first, just let me know). Thanks :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739328787
I did not differentiate https from http in any way in terms of identifying itself as http and it lives in the same module because it is literally the same code just applied to a different built-in module.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739078963
> Also, why are the meanings of Span.inject() and Span.extract() reversed in this agent compared to the Python one?
This is a stupid mistake I made, it should have had the same meanings as the Python agent’s.
> Would be good to have a chat, this agent was in very poor shape and I could use some clarification to make it work.
This agent indeed lacked of a lot of efforts, would be happy to have contributors like you to help to make it better. Feel free to ask questions in Slack channel or GitHub issues here, or mailing list. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739075323
> Config or resource issue?
Network from GitHub Actions runtime to hub.docker is unstable, have rerun it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel edited a comment on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740129916
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739087489
> > This agent indeed lacked of a lot of efforts, would be happy to have contributors like you to help to make it better. Feel free to ask questions in Slack channel or GitHub issues here, or mailing list. Thanks
>
>
>
> Which one will give me the quickest replies?
I check GitHub notifications most frequently, and reply as soon as I saw them, but due to the different time zones, I might wake up to check them in hours after your posts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739256071
Well the code for this done, it still needs a test and a Component ID allocated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740297329
> > Yes I can help on this, when you think it's all set except for the test, ping me I'll push to this branch directly to verify the plugin (or you'd like to merge this first, just let me know). Thanks :)
>
> Express and https should be good to go so do in whatever order you thinks makes sense.
Ok, well I am getting a little farther ahead than I thought and this PR is getting bigger, as well as a parallel branch I have for our own stuff which is getting more and more different. So maybe it would be better if you could merge this PR before anything else?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on a change in pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#discussion_r536927702
##########
File path: src/trace/context/SpanContext.ts
##########
@@ -57,39 +58,62 @@ export default class SpanContext implements Context {
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- const span = new EntrySpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- operation,
- });
+ let span;
- if (carrier && carrier.isValid()) {
- span.inject(carrier);
+ if (parent && parent.type === SpanType.ENTRY) {
+ span = parent;
+ parent.operation = operation;
+
+ } else {
+ span = new EntrySpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ operation,
+ });
+
+ if (carrier && carrier.isValid()) {
+ span.extract(carrier);
+ }
}
return span;
}
- newExitSpan(operation: string, peer: string): Span {
+ newExitSpan(operation: string, peer: string, carrier?: ContextCarrier): Span {
if (logger.isDebugEnabled()) {
logger.debug('Creating exit span', {
parentId: this.parentId,
executionAsyncId: executionAsyncId(),
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- return new ExitSpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- peer,
- operation,
- });
+ let span;
+
+ if (parent && parent.type === SpanType.EXIT) {
+ span = parent;
+
+ } else {
+ span = new ExitSpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ peer,
+ operation,
+ });
+
+ // if (carrier && carrier.isValid()) { // is this right?
+ // Object.assign(carrier, span.inject());
Review comment:
Hmm, sorry I was wrong, let me check a little more
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739246681
`2020-12-05T12:43:00.079Z testcontainers ERROR Failed to start DockerCompose environment: Creating network "testcontainers-28a887eed5d9791e474a4847a6654043_traveling-light" with the default driver`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739314133
> Well the code for this done, it still needs a test and a Component ID allocated.
The Component ID is allocated [here](https://github.com/apache/skywalking/blob/83dd23977dff90f22a7bea30b04bcfcfc013e8ec/oap-server/server-bootstrap/src/main/resources/component-libraries.yml#L455-L478)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739314772
Hmm, express was already there so I will just use that I guess. As for tests, I think it is something that would maybe take you 5 minutes but could take me hours to figure out your setup, any chance you could do the test for this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739437884
e
> I did not differentiate https from http in any way in terms of identifying itself as http and it lives in the same module because it is literally the same code just applied to a different built-in module.
+1 from me
>
> Also, I noticed there exists a specific component definition for NodeJS http server:
>
> ```yaml
> HttpServer:
> id: 4001
> languages: Node.js
> ```
This was originally for http://github.com/SkyAPM/SkyAPM-nodejs/
> But the one actually used in the http plugin is #49:
>
> ```yaml
> http:
> id: 49
> languages: Java,C#,Node.js
> ```
>
> And the client used is # 2.
> Which way do you guys want to go with this, more specific for the individual languages or should all language-common base protocols be the same (# 2 and # 49)?
Basically the latter one applies. The component IDs are only used to match the logo/icon in the web UI, if there is no logo/icon for that (e.g. most built-in modules), we just reuse the existing ID from Java language that shares the same protocol (e.g, http)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-738992841
Config or resource issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel edited a comment on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740004912
Some small fixes and one big fix - making the `DummySpan` actually work. Added suffix and path ignore mechanism via `SW_IGNORE_SUFFIX` and `SW_TRACE_IGNORE_PATH`. There is no `SW_TRACE_IGNORE` like in the Python agent, instead the presence of a non-empty `SW_TRACE_IGNORE_PATH` enables path ignore. I will probably change it in the Python agent to work this was if you have no objections (cleaner, fewer env vars to deal with).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740004912
Some small fixes here but mostly added suffix and path ignore mechanism via `SW_IGNORE_SUFFIX` and `SW_TRACE_IGNORE_PATH`. There is no `SW_TRACE_IGNORE` like in the Python agent, instead the presence of a non-empty `SW_TRACE_IGNORE_PATH` enables path ignore. I will probably change it in the Python agent to work this was if you have no objections (cleaner, fewer env vars to deal with).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel edited a comment on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739328787
I did not differentiate https from http in any way in terms of identifying itself as http and it lives in the same module because it is literally the same code just applied to a different built-in module.
Also, I noticed there exists a specific component definition for NodeJS http server:
```yml
HttpServer:
id: 4001
languages: Node.js
```
But the one actually used in the http plugin is #49:
```yml
http:
id: 49
languages: Java,C#,Node.js
```
And the client used is # 2.
Which way do you guys want to go with this, more specific for the individual languages or should all language-common base protocols be the same (# 2 and # 49)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel edited a comment on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740129916
This interacts with http plugin below it just like everything else, and event though they behave well together I am not completely happy with how this works right now. I think it can almost certainly be made more efficient and the interactions better defined (the main thing was just to get it working now, needs testing).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-740344768
> > > Yes I can help on this, when you think it's all set except for the test, ping me I'll push to this branch directly to verify the plugin (or you'd like to merge this first, just let me know). Thanks :)
> >
> >
> > Express and https should be good to go so do in whatever order you thinks makes sense.
>
> Ok, well I am getting a little farther ahead than I thought and this PR is getting bigger, as well as a parallel branch I have for our own stuff which is getting more and more different. So maybe it would be better if you could merge this PR before anything else?
Sure. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on a change in pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#discussion_r536926354
##########
File path: src/trace/context/SpanContext.ts
##########
@@ -57,39 +58,62 @@ export default class SpanContext implements Context {
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- const span = new EntrySpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- operation,
- });
+ let span;
- if (carrier && carrier.isValid()) {
- span.inject(carrier);
+ if (parent && parent.type === SpanType.ENTRY) {
+ span = parent;
+ parent.operation = operation;
+
+ } else {
+ span = new EntrySpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ operation,
+ });
+
+ if (carrier && carrier.isValid()) {
+ span.extract(carrier);
+ }
}
return span;
}
- newExitSpan(operation: string, peer: string): Span {
+ newExitSpan(operation: string, peer: string, carrier?: ContextCarrier): Span {
if (logger.isDebugEnabled()) {
logger.debug('Creating exit span', {
parentId: this.parentId,
executionAsyncId: executionAsyncId(),
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- return new ExitSpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- peer,
- operation,
- });
+ let span;
+
+ if (parent && parent.type === SpanType.EXIT) {
+ span = parent;
+
+ } else {
+ span = new ExitSpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ peer,
+ operation,
+ });
+
+ // if (carrier && carrier.isValid()) { // is this right?
+ // Object.assign(carrier, span.inject());
Review comment:
You don't need to do anything here, context carriers from exit span are meant to be sent to remote peer, so the plugin (or user codes) should tackle this and propagate them to remote peer (via http headers or MQ metadata, etc.) properly, like the http plugin
```nodejs
span.inject().items.forEach((item) => {
request.setHeader(item.key, item.value);
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on a change in pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#discussion_r536822567
##########
File path: src/trace/context/SpanContext.ts
##########
@@ -57,39 +58,62 @@ export default class SpanContext implements Context {
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- const span = new EntrySpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- operation,
- });
+ let span;
- if (carrier && carrier.isValid()) {
- span.inject(carrier);
+ if (parent && parent.type === SpanType.ENTRY) {
+ span = parent;
+ parent.operation = operation;
+
+ } else {
+ span = new EntrySpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ operation,
+ });
+
+ if (carrier && carrier.isValid()) {
+ span.extract(carrier);
+ }
}
return span;
}
- newExitSpan(operation: string, peer: string): Span {
+ newExitSpan(operation: string, peer: string, carrier?: ContextCarrier): Span {
if (logger.isDebugEnabled()) {
logger.debug('Creating exit span', {
parentId: this.parentId,
executionAsyncId: executionAsyncId(),
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- return new ExitSpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- peer,
- operation,
- });
+ let span;
+
+ if (parent && parent.type === SpanType.EXIT) {
+ span = parent;
+
+ } else {
+ span = new ExitSpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ peer,
+ operation,
+ });
+
+ // if (carrier && carrier.isValid()) { // is this right?
+ // Object.assign(carrier, span.inject());
Review comment:
Forgot this detail, what is the proper way to do the injection in `SpanContext.newExitSpan()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739080604
> This agent indeed lacked of a lot of efforts, would be happy to have contributors like you to help to make it better. Feel free to ask questions in Slack channel or GitHub issues here, or mailing list. Thanks
Which one will give me the quickest replies?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 merged pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#issuecomment-739439171
> As for tests, I think it is something that would maybe take you 5 minutes but could take me hours to figure out your setup, any chance you could do the test for this?
Yes I can help on this, when you think it's all set except for the test, ping me I'll push to this branch directly to verify the plugin. Thanks :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on a change in pull request #9: [WIP] express plugin, make spans actually work
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#discussion_r536927702
##########
File path: src/trace/context/SpanContext.ts
##########
@@ -57,39 +58,62 @@ export default class SpanContext implements Context {
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- const span = new EntrySpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- operation,
- });
+ let span;
- if (carrier && carrier.isValid()) {
- span.inject(carrier);
+ if (parent && parent.type === SpanType.ENTRY) {
+ span = parent;
+ parent.operation = operation;
+
+ } else {
+ span = new EntrySpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ operation,
+ });
+
+ if (carrier && carrier.isValid()) {
+ span.extract(carrier);
+ }
}
return span;
}
- newExitSpan(operation: string, peer: string): Span {
+ newExitSpan(operation: string, peer: string, carrier?: ContextCarrier): Span {
if (logger.isDebugEnabled()) {
logger.debug('Creating exit span', {
parentId: this.parentId,
executionAsyncId: executionAsyncId(),
});
}
- ContextManager.spansDup();
+ const spans = ContextManager.spansDup();
+ const parent = spans[spans.length - 1];
- return new ExitSpan({
- id: this.spanId++,
- parentId: this.parentId,
- context: this,
- peer,
- operation,
- });
+ let span;
+
+ if (parent && parent.type === SpanType.EXIT) {
+ span = parent;
+
+ } else {
+ span = new ExitSpan({
+ id: this.spanId++,
+ parentId: this.parentId,
+ context: this,
+ peer,
+ operation,
+ });
+
+ // if (carrier && carrier.isValid()) { // is this right?
+ // Object.assign(carrier, span.inject());
Review comment:
Hmm, sorry I was wrong, let me check a little more
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org