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

Open Telemetry #2706

Merged
merged 15 commits into from
Jan 29, 2024
Merged

Open Telemetry #2706

merged 15 commits into from
Jan 29, 2024

Conversation

dvoet
Copy link
Contributor

@dvoet dvoet commented Jan 18, 2024

Ticket: https://broadworkbench.atlassian.net/browse/WOR-1462

updates to use open telemetry
updates to use jakarta client libs

Most of the changes are minor. A few places added trace contexts where they were missing. A new RawlsTracingContext class used when a user is not present.


PR checklist

  • Include the JIRA issue number in the PR description and title
  • Make sure Swagger is updated if API changes
    • ...and Orchestration's Swagger too!
  • If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
  • Get two thumbsworth of PR review
  • Verify all tests go green, including CI tests
  • Squash commits and merge to develop (branches are automatically deleted after merging)
  • Inform other teams of any substantial changes via Slack and/or email

// SdkMeterProvider.builder
// .registerMetricReader(prometheusHttpServer)
// .setResource(resource)
// .build
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 am leaving these comments in here for now because I think we should switch to open telemetry for metrics reporting too

Copy link
Contributor

Choose a reason for hiding this comment

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

Is turning this on mutually exclusive with switching over the metrics reporting stuff? I'm wondering if this can be a gradual switchover with the prometheus server being on but exposing nothing, and then moving metrics over more and more until we can fully deactivate the statsd sidecar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can be gradual with the statsd stuff. Unfortunately the old OpenTelemetryMetrics class from workbench-libs (which actually uses open census) is in use and sitting on the prometheus port

httpRequestTimer(ExpandedMetricBuilder.empty)(request, response).update(elapsed, TimeUnit.MILLISECONDS)
response
def instrumentRequest: Directive1[Context] =
(traceRequest & extractRequest).tflatMap { case (otelContext, request) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that this is where tracing starts

@dvoet
Copy link
Contributor Author

dvoet commented Jan 24, 2024

jenkins retest

// SdkMeterProvider.builder
// .registerMetricReader(prometheusHttpServer)
// .setResource(resource)
// .build
Copy link
Contributor

Choose a reason for hiding this comment

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

Is turning this on mutually exclusive with switching over the metrics reporting stuff? I'm wondering if this can be a gradual switchover with the prometheus server being on but exposing nothing, and then moving metrics over more and more until we can fully deactivate the statsd sidecar.

val workspaceManager = clientLibExclusions("bio.terra" % "workspace-manager-client" % "0.254.997-SNAPSHOT")
val dataRepo = clientLibExclusions("bio.terra" % "datarepo-jakarta-client" % "1.568.0-SNAPSHOT")
val resourceBufferService = clientLibExclusions("bio.terra" % "terra-resource-buffer-client" % "0.198.42-SNAPSHOT")
val billingProfileManager = clientLibExclusions("bio.terra" % "billing-profile-manager-client" % "0.1.502-SNAPSHOT")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, we can get rid of the bpm and WSM javax clients once this rolls out.

Copy link
Contributor

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

couple comments inline, proactive 👍

)

try
traceNakedWithParent("executeGoogleRequest", childContext) { _ =>
Copy link
Contributor

Choose a reason for hiding this comment

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

does this lose the googleProjectId and billingAccount attributes on the span?

// WithOtelContextFilter must run before JakartaTracingFilter so give it a higher priority
val priority = 1
client.getHttpClient.register(new WithOtelContextFilter(otelContext), priority)
client.getHttpClient.register(new JakartaTracingFilter(GlobalOpenTelemetry.get()), priority + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this repeated a few times; is it worth a reusable method?

@dvoet
Copy link
Contributor Author

dvoet commented Jan 29, 2024

jenkins retest

1 similar comment
@dvoet
Copy link
Contributor Author

dvoet commented Jan 29, 2024

jenkins retest

@dvoet dvoet merged commit 4fae524 into develop Jan 29, 2024
10 of 11 checks passed
@dvoet dvoet deleted the otel branch January 29, 2024 18:47
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