refactor!: prometheus attributes format to otel attributes #4122
Conversation
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
02c2236
to
585a57a
Compare
|
@sergiuszkierat I just worked this a few days ago and haven't pushed here :) |
|
Just need tweaks here to this PR. Once it is done I will tag you here |
@varshith257 Thanks for letting me know. Better late than never 😅 I can freely switch to another task |
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
585a57a
to
9fd29dc
Compare
|
@adamw It's ready for your review |
server/core/src/main/scala/sttp/tapir/server/metrics/Metric.scala
Outdated
Show resolved
Hide resolved
server/core/src/main/scala/sttp/tapir/server/metrics/Metric.scala
Outdated
Show resolved
Hide resolved
...ry-metrics/src/main/scala/sttp/tapir/server/metrics/opentelemetry/OpenTelemetryMetrics.scala
Show resolved
Hide resolved
|
Will definitely look into it
…On Thu, 31 Oct, 2024, 2:41 pm Sergiusz Kierat, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
metrics/opentelemetry-metrics/src/main/scala/sttp/tapir/server/metrics/opentelemetry/OpenTelemetryMetrics.scala
<#4122 (comment)>:
> @@ -80,7 +80,7 @@ object OpenTelemetryMetrics {
def requestActive[F[_]](meter: Meter, labels: MetricLabels): Metric[F, LongUpDownCounter] =
Metric[F, LongUpDownCounter](
meter
- .upDownCounterBuilder("request_active")
+ .upDownCounterBuilder("http.server.active_requests")
are all metrics that we should report, included here, or is there anything
missing?
I guess that having those optional metrics (http.server.request.body.size
<https://opentelemetry.io/docs/specs/semconv/http/http-metrics/#metric-httpserverrequestbodysize>
,
http.server.response.body.size
<https://opentelemetry.io/docs/specs/semconv/http/http-metrics/#metric-httpserverresponsebodysize>)
would be useful 😁
—
Reply to this email directly, view it on GitHub
<#4122 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A4BF3HCGZPMQ2TVJIAY4MP3Z6HX4JAVCNFSM6AAAAABQ2Q6GUGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMBXGQZDMMJXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
453389b
to
ca9550f
Compare
varshith257
changed the title
server/core/src/main/scala/sttp/tapir/server/metrics/Metric.scala
Outdated
Show resolved
Hide resolved
server/core/src/main/scala/sttp/tapir/server/metrics/Metric.scala
Outdated
Show resolved
Hide resolved
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
38bf6fd
to
b433cda
Compare
|
@adamw Done! |
| }, | ||
| "error.type" -> { | ||
| case Left(ex) => ex.getClass.getSimpleName | ||
| case Right(_) => "" |
There was a problem hiding this comment.
I think if the response is a Right we shouldn't add any attributes
There was a problem hiding this comment.
Yes, it will helps.Will make changes for it
There was a problem hiding this comment.
@adamw The scala compiler is hungry to handle Right case?
WDYT the better way what you are thinking of?
There was a problem hiding this comment.
ah, well I guess this would have to return an Option[String], not a String, and then you would report only the defined labels
There was a problem hiding this comment.
@adamw It's getting a bit tricky making "error.type" to Option[String]. In that case, if we make forResponse then h http.response.status_code could go None for bad req?
There was a problem hiding this comment.
I made this comment 3 days before but don't know why not posted here lol
There was a problem hiding this comment.
yes, if an attribute is None we don't include it, if it's a Some we do
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
e4449d4
to
bd34265
Compare
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
bd34265
to
325a66b
Compare
varshith257
force-pushed
the
refactor/OTEL-attributes
branch
from
325a66b
to
ba7b18f
Compare
...ry-metrics/src/main/scala/sttp/tapir/server/metrics/opentelemetry/OpenTelemetryMetrics.scala
Outdated
Show resolved
Hide resolved
|
@adamw Can you approve workflows? I have no access to my desktop for now and I have made this commit through mobile :) |
...ry-metrics/src/main/scala/sttp/tapir/server/metrics/opentelemetry/OpenTelemetryMetrics.scala
Outdated
Show resolved
Hide resolved
/claim #4057
Closes #4057