-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Micrometer observations #1828
Micrometer observations #1828
Conversation
Bumps [json](https://github.com/douglascrockford/JSON-java) from 20220320 to 20220924. - [Release notes](https://github.com/douglascrockford/JSON-java/releases) - [Changelog](https://github.com/stleary/JSON-java/blob/master/docs/RELEASES.md) - [Commits](https://github.com/douglascrockford/JSON-java/commits) --- updated-dependencies: - dependency-name: org.json:json dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps `slf4j.version` from 2.0.2 to 2.0.3. Updates `slf4j-simple` from 2.0.2 to 2.0.3 - [Release notes](https://github.com/qos-ch/slf4j/releases) - [Commits](qos-ch/slf4j@v_2.0.2...v_2.0.3) Updates `slf4j-nop` from 2.0.2 to 2.0.3 - [Release notes](https://github.com/qos-ch/slf4j/releases) - [Commits](qos-ch/slf4j@v_2.0.2...v_2.0.3) Updates `slf4j-api` from 2.0.2 to 2.0.3 - [Release notes](https://github.com/qos-ch/slf4j/releases) - [Commits](qos-ch/slf4j@v_2.0.2...v_2.0.3) Updates `slf4j-jdk14` from 2.0.2 to 2.0.3 - [Release notes](https://github.com/qos-ch/slf4j/releases) - [Commits](qos-ch/slf4j@v_2.0.2...v_2.0.3) --- updated-dependencies: - dependency-name: org.slf4j:slf4j-simple dependency-type: direct:development update-type: version-update:semver-patch - dependency-name: org.slf4j:slf4j-nop dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.slf4j:slf4j-api dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.slf4j:slf4j-jdk14 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps `kotlin.version` from 1.7.10 to 1.7.20. Updates `kotlin-stdlib-jdk8` from 1.7.10 to 1.7.20 - [Release notes](https://github.com/JetBrains/kotlin/releases) - [Changelog](https://github.com/JetBrains/kotlin/blob/v1.7.20/ChangeLog.md) - [Commits](JetBrains/kotlin@v1.7.10...v1.7.20) Updates `kotlin-reflect` from 1.7.10 to 1.7.20 - [Release notes](https://github.com/JetBrains/kotlin/releases) - [Changelog](https://github.com/JetBrains/kotlin/blob/v1.7.20/ChangeLog.md) - [Commits](JetBrains/kotlin@v1.7.10...v1.7.20) Updates `kotlin-maven-plugin` from 1.7.10 to 1.7.20 --- updated-dependencies: - dependency-name: org.jetbrains.kotlin:kotlin-stdlib-jdk8 dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.jetbrains.kotlin:kotlin-reflect dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.jetbrains.kotlin:kotlin-maven-plugin dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps asm from 9.3 to 9.4. --- updated-dependencies: - dependency-name: org.ow2.asm:asm dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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've added small remarks but in general your suggestion looks much better than mine!
} | ||
|
||
String getMethodString(@Nullable Request request) { | ||
return request.httpMethod().name(); |
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.
If a request
is @Nullable
you might get NPE here (although I doubt that it would be the case in Feign)
* {@link ObservationConvention} for Feign. | ||
* | ||
* @since 1.10.0 | ||
* @see DefaultApacheHttpClientObservationConvention |
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.
Why this @see
? Also @since 1.10.0
seems like a copy from micrometer ;)
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'm not sure either, you wrote that =)
* A {@link SenderContext} for Feign. | ||
* | ||
* @author Marcin Grzejszczak | ||
* @since 1.10.0 |
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.
@since 13.0
I guess?
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.
FWIW, Feign next release going to be 12.1
/** | ||
* {@link ObservationDocumentation} for Feign. | ||
* | ||
* @since 1.10.0 |
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.
@since 13.0
?
STATUS { | ||
@Override | ||
public String asString() { | ||
return "status"; |
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've made a mistake in the original PR, let's go with the semantic conventions from OTel https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md
method
->http.method
status
->http.status_code
target.port
->net.peer.port
target.host
->net.peer.name
uri
->http.url
I don't see scheme
in the conventions so we can go with http.scheme
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.
What if you merge my chances into your PR and then you can do the required changes?
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.
No problem
/** | ||
* Default implementation of {@link FeignObservationConvention}. | ||
* | ||
* @since 1.10.0 |
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.
@since 13.0
?
Merged this PR to this one #1760 |
Alternative micrometer observation using capability