Skip to content

fix(tracing): should trace request stopped by extern plugin#6500

Merged
spacewander merged 4 commits into
apache:masterfrom
spacewander:cmh
Mar 6, 2022
Merged

fix(tracing): should trace request stopped by extern plugin#6500
spacewander merged 4 commits into
apache:masterfrom
spacewander:cmh

Conversation

@spacewander

Copy link
Copy Markdown
Member

Signed-off-by: spacewander [email protected]

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@spacewander

Copy link
Copy Markdown
Member Author

I don't change the skywalking/opentracing because increasing the priority will make the body_filter call earlier.

Can we remove the finish in the body_filter and only finish the span in the log phase?
@dmsolr @yangxikun
Please guide me the right way.

@spacewander
spacewander marked this pull request as ready for review March 3, 2022 07:32
Signed-off-by: spacewander <[email protected]>
@spacewander

spacewander commented Mar 3, 2022

Copy link
Copy Markdown
Member Author

Or maybe we can switch to an onion model for the filter?(Not a good idea)

@dmsolr

dmsolr commented Mar 4, 2022

Copy link
Copy Markdown
Member

I think finish() should be after ext-plugin, but start() should be before ext-plugin.
Moving the finish() to the log phase represents increasing the response time in trace view.

the trace includes 2 spans in APISIX,
the first span, entry span, is over the whole request. It means from the request accepted to the response returned.
the 2nd span, exit span, is only from the sending request to accepting the response.
(Now, those spans start and stop at the same time.)

Signed-off-by: spacewander <[email protected]>
@spacewander

Copy link
Copy Markdown
Member Author

I will open another issue to trace #6500 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants