-
Notifications
You must be signed in to change notification settings - Fork 787
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 support for InfluxDB #10850
Add support for InfluxDB #10850
Conversation
.../java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbAttributesGetter.java
Show resolved
Hide resolved
.../java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbAttributesGetter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbConstants.java
Outdated
Show resolved
Hide resolved
...va/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java
Outdated
Show resolved
Hide resolved
...c/test/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClientTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClientTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClientTest.java
Outdated
Show resolved
Hide resolved
new Query( | ||
"SELECT MEAN(water_level) FROM h2o_feet where time = '2022-01-01T08:00:00Z'; SELECT water_level FROM h2o_feet LIMIT 2", | ||
databaseName); | ||
influxDb.query( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we test for apis that have that kind of callbacks is that the callback also runs with expected context. To do this we add a span named parent
around the operation we are going to test and create a span named child
in the callback. Span from callback should be a sibling for the span for the executed operation. See
Line 148 in 42ba962
void requestWithCustomInterceptor() throws Throwable { |
"child"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we test for apis that have that kind of callbacks is that the callback also runs with expected context. To do this we add a span named
parent
around the operation we are going to test and create a span namedchild
in the callback. Span from callback should be a sibling for the span for the executed operation. SeeLine 148 in 42ba962
void requestWithCustomInterceptor() throws Throwable { for an example, you can find more samples by searching for
"child"
.
I added relevant callback tests. But I found because of the implement of callback logic in https://github.com/influxdata/influxdb-java/blob/0c690c8d8a6627f525e05d68e0571e089c61845e/src/main/java/org/influxdb/impl/InfluxDBImpl.java#L637 the child
span is the child of the executed operation instead of sibling:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented running callbacks in the scope of the parent span.
trace -> | ||
trace.hasSpansSatisfyingExactly( | ||
span -> | ||
span.hasName("write unknown") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming rules for database client span names in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md do not suggest using unknown
when database name is not known, they suggest to omit the database name instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming rules for database client span names in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md do not suggest using
unknown
when database name is not known, they suggest to omit the database name instead.
Thank you for your reminder, I modified it now. But I found in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md, it is not clearly stated that it is recommended to omit the database name in this case. Is it necessary to add this situation in the document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reread this part in the spec and noticed that it was changed a couple of days ago. Now the spec suggests a prioritized list of attributes that could be added to the span name. We have not implemented this yet.
Repen to trigger CI tasks. |
Resolves #1572