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

align elasticsearch body capturing with other agents #1013

Merged
merged 6 commits into from Jan 28, 2021

Conversation

beniwohli
Copy link
Contributor

See elastic/apm#55

This also allows us to get rid of a hack we used to pass the API method
and an unserialized version of the body to the instrumentation. Neither
is used anymore, so we can skip that part of the instrumentation.

This does also mean that we stop capturing the body for some methods,
specifically update and delete_by_query.

closes #419

See elastic/apm#55

This also allows us to get rid of a hack we used to pass the API method
and an unserialized version of the body to the instrumentation. Neither
is used anymore, so we can skip that part of the instrumentation.

This does also mean that we stop capturing the body for some methods,
specifically `update` and `delete_by_query`.

closes elastic#419
@beniwohli
Copy link
Contributor Author

@basepi @felixbarny I guess strictly speaking this could constitute a backwards incompatible change, so I vote for including this in 6.0, not 5.10.1. What do you think?

@felixbarny
Copy link
Member

@trentm has been advocating for having the list of endpoints that we capture bodies for be configurable.

This does also mean that we stop capturing the body for some methods,
specifically update and delete_by_query.
I guess strictly speaking this could constitute a backwards incompatible change

Would the backwards compatibility be remained if you just included */delete_by_query and */_update/* to the URL patterns that have body capturing enabled?
This would enable us to merge the new code in now as opposed to the 6.x branch at the expense of being slightly out-of-sync with what the other agents are doing. But in 6.0, we could just remove the patterns again or decide to add them to the default lists of other agents.

For these things, I sometimes struggle to find a clear border of what's considered backwards incompatible. In general, I think we should be able to fine-tune the amount of data we collect without having to major-bump the agent version.
Maybe in this case it's fine to remove the delete_by_query and update APIs from the default list as long as there's a config option for users to re-enable them.

@apmmachine
Copy link
Collaborator

apmmachine commented Jan 20, 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

Expand to view the summary

Build stats

  • Build Cause: Started by user Colton Myers

    • Start Time: 2021-01-28T17:36:35.274+0000
  • Duration: 21 min 34 sec

  • Commit: 43dce2d

Test stats 🧪

Test Results
Failed 0
Passed 8412
Skipped 5888
Total 14300

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 8412
Skipped 5888
Total 14300

@beniwohli
Copy link
Contributor Author

@felixbarny 6.0 isn't very far away, probably just a couple of weeks.

As for including those two endpoints, at least for update, we previously only captured the script part of the body (for reasons that I'm sure made sense at the time). If at all possible, I want to avoid adding that back, since we then need to introduce that ugly hack again (or re-parse the already serialized JSON body).

@felixbarny
Copy link
Member

6.0 isn't very far away, probably just a couple of weeks.

ah, nvm then

using params=None never made sense here
@basepi basepi added this to Planned in APM-Agents (OLD) via automation Jan 28, 2021
@basepi basepi moved this from Planned to In Progress in APM-Agents (OLD) Jan 28, 2021
@basepi basepi added this to the 6.0 milestone Jan 28, 2021
@basepi basepi merged commit d2e9cd4 into elastic:master Jan 28, 2021
APM-Agents (OLD) automation moved this from In Progress to Done Jan 28, 2021
beniwohli added a commit to beniwohli/apm-agent-python that referenced this pull request Sep 14, 2021
* align elasticsearch body capturing with other agents

See elastic/apm#55

This also allows us to get rid of a hack we used to pass the API method
and an unserialized version of the body to the instrumentation. Neither
is used anymore, so we can skip that part of the instrumentation.

This does also mean that we stop capturing the body for some methods,
specifically `update` and `delete_by_query`.

closes elastic#419

* fix test code

using params=None never made sense here

* update path matching regex to align with Node.js agent

* Add CHANGELOG

Co-authored-by: Colton Myers <colton.myers@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Capture request body for more Elasticsearch APIs
5 participants