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

WOR-1510 upgrade otel override HttpServerMetrics #131

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

dvoet
Copy link
Contributor

@dvoet dvoet commented Feb 8, 2024

Copy link

sonarqubecloud bot commented Feb 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dvoet dvoet changed the title upgrade otel override HttpServerMetrics WOR-1510 upgrade otel override HttpServerMetrics Feb 8, 2024
Comment on lines +30 to +43
d -> {
if (d < .5) {
return d + 0.01;
}
if (d < 2) {
return d + 0.1;
}
if (d < 10) {
return d + 1;
}
if (d < 60) {
return d + 5;
}
return d + 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is Java 17, this could be a switch with guards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copilot wrote this code :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🚫

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think guards is a preview feature of java 17 and we don't have that enabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, darn

Copy link
Contributor

@okotsopoulos okotsopoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as far as I can tell.

Does a service using OTEL via TCL need to do anything extra to get these changes? Is there anywhere I can see these updated metrics in action?

@dvoet
Copy link
Contributor Author

dvoet commented Feb 9, 2024

Does a service using OTEL via TCL need to do anything extra to get these changes?

yes, they need to upgrade their TCL version, I will start doing that tomorrow

@dvoet dvoet merged commit 1304f15 into develop Feb 9, 2024
2 checks passed
@dvoet dvoet deleted the otel_update branch February 9, 2024 01:34
okotsopoulos added a commit to DataBiosphere/terra-workspace-manager that referenced this pull request Mar 5, 2024
./gradlew service:bootRun was failing out of the gate with java.lang.NoClassDefFoundError: io/opentelemetry/instrumentation/api/internal/SemconvStability
I updated dependencies using DataBiosphere/terra-common-lib#131 as a guide (which is included in this TCL version bump).
Is there a way to inherit these OTEL dependencies from TCL?
okotsopoulos added a commit to DataBiosphere/terra-workspace-manager that referenced this pull request Mar 7, 2024
This required OTEL dependency upgrades for bootRun to work, following DataBiosphere/terra-common-lib#131
Is there a better way to get these from TCL without redefining them downstream?
okotsopoulos added a commit to DataBiosphere/terra-workspace-manager that referenced this pull request Mar 20, 2024
* Remove GcpMetricsExporter and FeatureService flag

This is now enabled as needed via TCL's terra.common.tracing.stackdriverExportEnabled property.

* Remove Flagsmith from FeatureService

TCL upgrade will have removed it.  All of our AWS functionality is unsupported, so default to disabled.
Ticket to fully clean up FeatureService and related code: WOR-1559

* Upgrade terra-common-lib to 1.0.9

This required OTEL dependency upgrades for bootRun to work, following DataBiosphere/terra-common-lib#131
Is there a better way to get these from TCL without redefining them downstream?

* Leverage Stairway 1.0.0's native context-awareness

MdcHook is greatly reduced and renamed to StairwayLoggingHook: it now only logs at notable state transitions.

* Consistent com.google.cloud:libraries-bom version throughout

This now matches the integration version as well as the version in TCL.

* Remove now-unused MDCHandlingException

* Streamline opentelemetry dependencies

I learned that Spring Boot Dependency Manager pulls in opentelemetry-bom as a dependency.
For our Spring Boot version 3.1.2, opentelemetry-bom has version 1.25.0.
Using the BOM (bill of materials) to manage OTEL dependencies is preferred to managing them individually: the BOM makes sure that they remain compatible with one another.
The only OTEL dependencies that WSM needs to define directly are:
1. opentelemetry-api so that we can construct OpenTelemetry objects (versioned by the BOM, which is itself versioned by Spring Boot dependency manager)
2. opentelemetry-instrumentation-annotations so that we can use the @WithSpan annotation

terra-common-lib defines the OTEL deps it needs, they are available as runtime dependencies so we don't need to redefine them.
But it does not use the BOM to version them, or Spring Boot dependency manager.  It versions them directly at 1.34.1.
I found when removing the OTEL deps that WSM didn't need, I also needed to instruct our Spring Boot dependency manager to pin opentelemetry-bom at a higher version
compatible with TCL's OTEL dependencies.

* Consistent Google BOM versioning

After merging in the main branch, I noticed that our Google BOM versions have again diverged.
There is a wider mystery here as to why Dependabot is not picking up on some dependencies, but for now, bringing this one up to date.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants