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

Make sure elasticsearch instrumentation follows semantic conventions #1633

Open
Tracked by #1624
srikanthccv opened this issue Feb 4, 2023 · 2 comments
Open
Tracked by #1624
Labels
good first issue Good for newcomers help wanted Extra attention is needed instrumentation

Comments

@srikanthccv
Copy link
Member

No description provided.

@remram44
Copy link
Contributor

remram44 commented Feb 4, 2023

Re-posting the details here, since you closed the earlier issue in favor of a later one with a blank description.

A bit rude to be honest, you make contributors fill out an extensive template, and then you overrule them with empty tickets.


Describe your environment
Using opentelemetry-instrumentation-elasticsearch 0.24b0 with elasticsearch 7.14.1

What is the expected behavior?
The instrumentation should follow the semantic conventions for database clients, notably use span names in the format <db.operation> <db.name>.<db.sql.table>.

What is the actual behavior?
Span names are built from the request URL without any form of parsing. In particular, the operation (index/delete/...) is missing, and the database name is not necessarily present.

Example span names:

  • Elasticsearch/indexname/_doc/documentid (insert operation if PUT, read operation if GET)
  • Elasticsearch/indexname/_doc (probably an insert without a set primary key)
  • Elasticsearch/indexname (either reading metadata or changing schema/settings), Elasticsearch/indexname/_search (query operation)

Many attributes are also missing or are only present in the request path, because they get encoded in the URL before perform_request() is called, which is the only instrumented method.

@srikanthccv
Copy link
Member Author

We appreciate all contributions. It's unfortunate if it came across as rude. It wasn't the intention to "overrule".

Here is our POV; with hundreds of issues in the backlog (nobody has time for a contrib repo), it's unrealistic to independently every issue to see if it's still relevant after dozen new releases and what parts are already solved or fixed. There were many similar issues about spec compliance with slight variations. Your issue had the linked PR, which seemed to address the part of it. The instrumentation version described is far behind, and we don't run es7 on CI and haven't decided what to do. It has often helped us close the old ones and ask for help. Simply copying the old text won't be much helpful because we are going to verify anyways what's missing as of today and devise a plan. And leaving behind many issues with the same goal won't make project management easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed instrumentation
Projects
None yet
Development

No branches or pull requests

2 participants