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

Add semantic convention of influxDB #949

Merged
merged 11 commits into from
May 29, 2024
Merged

Conversation

steverao
Copy link
Contributor

@steverao steverao commented Apr 23, 2024

Changes

Please provide a brief description of the changes here.
Add semantic convention of influxDB. Relative to open-telemetry/opentelemetry-java-instrumentation#10850

Merge requirement checklist

@steverao steverao changed the title Add semantic convertion of influxDB Add semantic convention of influxDB Apr 24, 2024
@steverao steverao marked this pull request as ready for review April 24, 2024 12:54
@steverao steverao requested review from a team April 24, 2024 12:54
.chloggen/influxdb.yaml Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor

@steverao do you envision adding any specifics about InfluxDB? Like custom attributes, metrics, some non-obvious mapping for common attributes?

If the goal is to add a constant for db.system, then it can be done without adding influxdb.md or the yaml section for it.
Curious what you think and what's your motivation to add things beyond system constant.
Thanks!

@steverao
Copy link
Contributor Author

@steverao do you envision adding any specifics about InfluxDB? Like custom attributes, metrics, some non-obvious mapping for common attributes?

At present, besides db.system, I also added some information about new database framework of InfluxDB, such as attributes and examples in https://github.com/open-telemetry/semantic-conventions/pull/949/files#diff-b0b92e2b139407d14dd05fec354af626034d748c5ced214a074e1e2389cffc2c

@lmolkova
Copy link
Contributor

lmolkova commented May 7, 2024

@steverao it does not look like influxDB needs any customization on top of generic attributes and descriptions we have for common DB attributes/spans. Am I correct?

There is no requirement to add a semconv file for each DB and I'd prefer if we just added a constant system name unless we have something custom.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 23, 2024
@steverao
Copy link
Contributor Author

it does not look like influxDB needs any customization on top of generic attributes and descriptions we have for common DB attributes/spans. Am I correct?

Yes, for the plugin, it don't contain special customization.

There is no requirement to add a semconv file for each DB.

At first, I saw other database plugins provide relevant YAML document, so I added it for InfluxDB. For this situation, don' you recommend to add its unique YAML document? In the document, I added some explanations for its attributes. I think it will be better if we conserve it. If you think it's not necessary. I can delete it later.

@lmolkova
Copy link
Contributor

@steverao

Thanks for the context!
We have ~50 different DB systems and only ~10 have a dedicated yaml/md. The criteria to add a yaml and md file if there are any custom attributes or some behavior that's different than one described in the common docs.

It seems that InfluxDB usage of common attributes is very well aligned with common definitions (database name is used in db.namespace and command name in the db.operation.name), so it should be enough just to add db.system constant.

@steverao
Copy link
Contributor Author

steverao commented May 27, 2024

It seems that InfluxDB usage of common attributes is very well aligned with common definitions (database name is used in db.namespace and command name in the db.operation.name), so it should be enough just to add db.system constant.

Thank you for your explanation! I deleted other contents except db.system constant in InfluxDB YAML document now. Please help me to review whether there are any other problems again?

@steverao
Copy link
Contributor Author

It seems there is unstable URL in other document which caused the CI running failed!
image

docs/database/influxdb.md Outdated Show resolved Hide resolved
@arminru arminru merged commit c98600c into open-telemetry:main May 29, 2024
12 checks passed
gregkalapos pushed a commit to gregkalapos/semantic-conventions that referenced this pull request May 29, 2024
Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
@steverao steverao deleted the influxdb branch May 30, 2024 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants