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

Http4s MetricsOps #46

Merged
merged 16 commits into from
May 28, 2024
Merged

Http4s MetricsOps #46

merged 16 commits into from
May 28, 2024

Conversation

lhns
Copy link
Contributor

@lhns lhns commented Jan 15, 2024

Hi, I implemented the http4s MetricsOps trait for otel4s.

  • Use names conforming to otel semconv
  • Is the core module the right place for a metrics integration or does it belong into a separate module?
  • How should the histogram buckets be configured? (fixed in otel4s 0.5.0-RC1)
  • Implement Test and Example
  • The PR is ready for review

Related: http4s/http4s#7448

@alexcardell
Copy link

Whether they should be separate middleware is an interesting question, as combining them (having a Tracer context bound for the metrics middleware) opens up the possibility of using exemplars

@NthPortal
Copy link
Contributor

honestly, I don't know enough to review this PR well. thoughts, @rossabaker ?

@lhns lhns marked this pull request as ready for review March 26, 2024 17:05
@iRevive
Copy link
Contributor

iRevive commented Mar 30, 2024

Whether they should be separate middleware is an interesting question, as combining them (having a Tracer context bound for the metrics middleware) opens up the possibility of using exemplars

I wonder if it will work in the current state.

OpenTelemetry Java uses JContext.current() to attach trace_id/span_id to an exemplar. The context cannot be propagated seamlessly between CE IOLocal and oteljava ThreadLocal. There are more details: typelevel/otel4s#202 and a possible fix typelevel/otel4s#214 (waiting for CE, though).

However, we can propagate the context manually (changes to otel4s required). For example, a Counter instrument:

def add(
  value: A,
  attributes: immutable.Iterable[Attribute[_]]
): F[Unit] =
  Ask[F, Context].ask.flatMap { ctx =>
    Sync[F].delay {
      val scope = ctx.underlying.makeCurrent() // make the current context active
      counter.add(cast(value), attributes.toJavaAttributes) 
      scope.close() // release the context
    }  
  }

@iRevive
Copy link
Contributor

iRevive commented Mar 30, 2024

Do you think the metric names are chosen correctly?

Here are the recommended names and dimensions: https://opentelemetry.io/docs/specs/semconv/http/http-metrics/.

I'm unsure whether the spec should be followed 1-to-1 though.

@iRevive
Copy link
Contributor

iRevive commented Mar 30, 2024

If we follow OpenTelemetry suggestions (i.e. attribute/dimension names) there would be major differences with the http4s-prometheus.

So it wouldn't work as a drop-in replacement (from the visualization standpoint, e.g. Grafana dashboard).

On the other hand, if we follow the suggestions, metrics provided by the http4s-otel4s should be compatible with any OpenTelemetry http dashboard (e.g. https://grafana.com/grafana/dashboards/18860-http-metrics-opentelemetry/). This is a plus when you monitor various services written in different languages/stacks.

@lhns
Copy link
Contributor Author

lhns commented Mar 30, 2024

I am pretty sold on otel and I think the most future proof thing would be to rely on their semconv. The naming already differs a bit from the prometheus impl because of the dots in the metric names. If we want to link traces and metrics they should probably follow the same naming conventions.

@lhns
Copy link
Contributor Author

lhns commented Mar 30, 2024

I now changed the metric names and attributes to use otel semantic naming where applicable. A few exceptions are:

  • http.server.abnormal_terminations
  • http.phase
  • classifier

@lhns lhns force-pushed the feature/otel-metrics branch 6 times, most recently from 7657c15 to f9a8552 Compare April 2, 2024 07:51
@lhns
Copy link
Contributor Author

lhns commented Apr 2, 2024

rebased on #65

@NthPortal
Copy link
Contributor

sorry for renaming and breaking everything >.>

@lhns
Copy link
Contributor Author

lhns commented Apr 3, 2024

I added the Metrics middleware to Http4sExample and created a small test. GH Actions seem to run fine on my fork.

@lhns
Copy link
Contributor Author

lhns commented Apr 12, 2024

@rossabaker just wanted to let you know that the PR should be ready for review. :)

@iRevive
Copy link
Contributor

iRevive commented May 6, 2024

Apologies for the git conflicts. Since otel4s 0.7.0, we can use otel4s-sdk-teskit to run tests across all platforms. Example.

Here is the branch with resolved conflicts: https://github.com/iRevive/http4s-otel4s-middleware/pull/1/files.

# Conflicts:
#	build.sbt
#	core/src/test/scala/org/http4s/otel4s/middleware/ClientMiddlewareTests.scala
@NthPortal
Copy link
Contributor

Since otel4s 0.7.0, we can use otel4s-sdk-teskit to run tests across all platforms.

the latest main (currently released as 0.6.0) has been updated to use otel4s 0.7.0

Copy link
Contributor

@iRevive iRevive 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!

Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

thanks for all the work you've put into this @lhns (and to @iRevive for the reviews)!

@NthPortal NthPortal merged commit 85c7af1 into http4s:main May 28, 2024
11 checks passed
@NthPortal
Copy link
Contributor

thanks again @lhns!

@NthPortal
Copy link
Contributor

release tagged as v0.7.0-rc.1 including this PR; it will be on maven central as soon as the build finishes and things propagate

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.

5 participants