Skip to content
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: impl contextPropagationOnly config var; change disableSend behaviour #2396

Merged
merged 5 commits into from
Nov 2, 2021

Conversation

trentm
Copy link
Member

@trentm trentm commented Oct 27, 2021

Implement contextPropagationOnly per spec (it does what disableSend
did before this change). Change disableSend to just disable comms
with APM server, but otherwise not attempt to reduce work.
https://github.com/elastic/apm/blob/master/specs/agents/transport.md#context_propagation_only-configuration

Closes: #2393

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

…viour

Implement contextPropagationOnly per spec (it does what disableSend
did before this change). Change disableSend to just disable comms
with APM server, but otherwise *not* attempt to reduce work.
https://github.com/elastic/apm/blob/master/specs/agents/transport.md#context_propagation_only-configuration
@trentm trentm requested review from astorm and mshustov October 27, 2021 20:46
@trentm trentm self-assigned this Oct 27, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 27, 2021
@trentm
Copy link
Member Author

trentm commented Oct 27, 2021

@mshustov As mentioned in the issue comment my hope is that if there is a Node.js APM agent release with this PR out before elastic/kibana#112973 and elastic/kibana#102704 go in, that the Kibana issue and pull can be updated to use the config var name contextPropagationOnly rather than disableSend.

The reason for the config var name change is that disableSend was implemented in the Node.js APM agent when it looked like the spec PR was going to settle on this name. But then it turned out the config var name disableSend is used in other language APM agents (Python and Ruby) to mean something slightly different, so for consistency the functionality Kibana wants here is now called contextPropagationOnly.

Is this acceptable to you?

(If there is a mix-up and Kibana uses disableSend with an APM agent including this PR change, then the only down side will be that the APM agent does some extra undesired processing -- like collecting error stacktraces and collecting metrics.)

@apmmachine
Copy link
Contributor

apmmachine commented Oct 27, 2021

💚 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

  • Start Time: 2021-11-02T16:04:52.008+0000

  • Duration: 22 min 49 sec

  • Commit: 55c95a7

Test stats 🧪

Test Results
Failed 0
Passed 22
Skipped 0
Total 22

🤖 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.

astorm
astorm previously approved these changes Oct 29, 2021
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Looks to do the thing it says it does. I see both disableSend and contextPropagationOnly will generate a NoOp transport, and contextPropagationOnly has a few extra guard clauses (replacing the old disableSend clauses) to prevent needed processing time. Config boiler plate looks to be setup correctly as well.

@mshustov
Copy link

mshustov commented Nov 2, 2021

@trentm sorry for the late response, I was vacationing. The proposed solution makes sense to me. elastic/kibana#112973 and elastic/kibana#102704 aren't going to be released in v8.0 anyway.

When are you going to release a client with these changes?
We can merge elastic/kibana#112973 and switch from disableSend to contextPropagationOnly as soon as the new version is available.

@trentm
Copy link
Member Author

trentm commented Nov 2, 2021

When are you going to release a client with these changes?

Either this week, or early next week. I'll ping on those two issues when we have a release with this in it. Thanks!

@trentm trentm merged commit ba570ba into master Nov 2, 2021
@trentm trentm deleted the trentm/context-propagation-only branch November 2, 2021 18:36
@trentm
Copy link
Member Author

trentm commented Nov 9, 2021

@mshustov FYI: elastic-apm-node@3.24.0 is now published which includes this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement contextPropagationOnly config var, tweak disableSend behaviour
4 participants