Skip to content

feat: mark Elasticsearch spans as exit spans; HTTP child spans are now elided#2557

Merged
trentm merged 5 commits intomainfrom
trentm/exit-spans-es
Feb 3, 2022
Merged

feat: mark Elasticsearch spans as exit spans; HTTP child spans are now elided#2557
trentm merged 5 commits intomainfrom
trentm/exit-spans-es

Conversation

@trentm
Copy link
Copy Markdown
Member

@trentm trentm commented Feb 2, 2022 •
edited
Loading

Elasticsearch (ES) client spans are no marked as exit spans, so their HTTP
child spans are now no longer generated per spec:
https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans.md#exit-spans

Some HTTP context has been added to ES spans:

  • span.context.http.status_code
  • span.context.http.response.encoded_body_size

Closes: #2000
Closes: #2484

Limitation

// - When using the non-default `asyncHooks=false` option with
//   @elastic/elasticsearch >=8 instrumentation, the diagnostic events do not
//   have the async run context of the current span. There are two impacts:
//   1. Elasticsearch tracing spans will not have additional "http" context
//      about the underlying HTTP request.
//   2. Users cannot access `apm.currentSpan` inside a diagnostic event handler.

I am currently inclined to not deal with this for asyncHooks=false. It is a minor degradation on generated ES spans (no span.context.http) and we never had a guarantee of the currentSpan being correct in the diagnostic events of the ES client before anyway. @astorm, please let me know if you disagree.

Checklist

  • Implement code
  • Add tests
  • Update documentation
  • Add CHANGELOG.asciidoc entry

@trentm
feat: mark Elasticsearch spans as exit spans; HTTP child spans are no…
a1c4c52 
… elided

Elasticsearch (ES) client spans are no marked as exit spans, so their HTTP
child spans are now no longer generated per spec:
https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans.md#exit-spans

Some HTTP context has been added to ES spans:
- span.context.http.status_code
- span.context.http.response.encoded_body_size

Closes: #2000
Closes: #2484
@trentm trentm self-assigned this Feb 2, 2022
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Feb 2, 2022
@ghost
Copy link
Copy Markdown

ghost commented Feb 2, 2022 •
edited by ghost
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Reason: null

  • Start Time: 2022-02-02T19:25:41.242+0000

  • Duration: 44 min 0 sec

  • Commit: af2ab95

Test stats 🧪

Test Results
Failed 0
Passed 240297
Skipped 0
Total 240297

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@trentm
Copy link
Copy Markdown
Member Author

trentm commented Feb 2, 2022

run module tests for elasticsearch,@elastic/elasticsearch,@elastic/elasticsearch-canary

@trentm
Copy link
Copy Markdown
Member Author

trentm commented Feb 2, 2022

run module tests for elasticsearch,@elastic/elasticsearch,@elastic/elasticsearch-canary

@trentm
Copy link
Copy Markdown
Member Author

trentm commented Feb 2, 2022

Note that tests actually all passed here: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-nodejs%2Fapm-agent-nodejs-mbp/detail/PR-2557/3/pipeline/1066

The error is:

[2022-02-02T19:17:20.473Z] Recording test results
[2022-02-02T19:17:21.399Z] null
[2022-02-02T19:17:21.129Z] No test report files were found. Configuration error?

on a TAV test step, which doesn't expect to provide TAP output. I don't know why that part of the Jenkinsfile reporting failed.

@trentm
Copy link
Copy Markdown
Member Author

trentm commented Feb 2, 2022

run module tests for elasticsearch,@elastic/elasticsearch,@elastic/elasticsearch-canary

@trentm trentm marked this pull request as ready for review February 2, 2022 19:25
@trentm trentm requested a review from astorm February 2, 2022 19:25
astorm
astorm previously approved these changes Feb 3, 2022
View reviewed changes
@trentm trentm merged commit 10f533e into main Feb 3, 2022
@trentm trentm deleted the trentm/exit-spans-es branch February 3, 2022 21:11
@trentm trentm mentioned this pull request Feb 10, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@astorm astorm astorm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@trentm trentm

Labels

agent-nodejs Make available for APM Agents project planning.

Projects

None yet

Milestone

No milestone

3 participants

@trentm @astorm @elastic-apm-tech