Don't create Elasticsearch span names containing document IDs

[remram44]

Description

This removes the unique document IDs from the span names, replacing them with ':id'. It greatly reduces the number of span names created by the elasticsearch intrumentation (e.g. no longer potentially infinite).

Note that this only works if the mapping type is _doc. Unfortunately elasticsearch index()/delete()/... uses _make_path() before calling perform_request() and this is not easily reversible. I believe this to be fine because mapping types are deprecated since Elasticsearch 7: https://www.elastic.co/guide/en/elasticsearch/reference/current/removal-of-types.html

Fixes #704

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Tested on my system with index and delete operations
• Ran tox -e py38-test-instrumentation-elasticsearch6

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[owais]

This feels like short term hack than a proper solution. We should try to update the instrumentation so we can follow the Otel conventions and use <db.operation> <db.name>.<index.name> as the span name.

[owais]

In case you cannot do that right now but need a fix urgently, you can use the request hook to provide a callback that updates the span name to whatever works for you.

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py#L52-L54

https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/span.py#L116

[remram44]

I feel like this is a serious problem affecting anyone using the Elasticsearch instrumentation (at least with non-autogenerated IDs). I was genuinely surprised to see no open issue about this. I find the instrumentation quite unusable when it will generate multiple new span names for every indexed document, even causing performance issues with the Jaeger web interface.

I am with you on the current code being quite limited, and this being a stopgap measure. However keeping the elasticsearch instrumentation in the repo while requiring all users to implement their own patch doesn't seem like it helps anyone.

Getting to the operation names you describe is essentially a full rewrite of the package. Not necessarily a difficult one, there are just many methods to instrument.

[owais]

OK. Can you open an issue and describe the long term solution there? Then add a TODO to your regex solution and link to the issue.

[remram44]

Done: created #708, mentioned it in a comment

[owais]

Thanks. Looking good. Please add a test case and fix lint issues (if any) and we can merge this.

[owais]

Also please add a changelog entry to CHANGELOG.md

[owais]

@remram44 can you please resolve the conflicts so we can merge this and include in the next release happening in 1-2 days?

[remram44]

Yes I will look into it!

[remram44]

The test failure (docker-tests) seems unrelated to my changes, it's broken on main branch

[remram44]

Rebased again