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

Add tracing to Memcached and Kafka operations #125

Merged
merged 9 commits into from
Nov 1, 2023
Merged

Conversation

pbrisbin
Copy link
Member

@pbrisbin pbrisbin commented Oct 25, 2023

Fix import order

b82b2d7

Add kind-related helpers for SpanArguments

02c522f

Use trace kind Client for runDB span

6ee57d5

Add tracing to Memcached operations

b4fe410

Add tracing to Kafka.Producer operations

59f17ae

This only adds topic name to the attributes, since how to deal with
multiple values and a keying function would be somewhat arbitrary. We'll
leave out that behavior for now and possibly add something later when we
start to use this and decide what we'd want.

Add tracing to Kafka.Consumer operations

d8ad053

This adds an outer span, and inner spans for decoding and handling the
message.

In order for spans to be correctly marked as error with exception
details, you need to "throw through" the inSpan function, so I had to
do that refactor too. This ultimately worked out simpler by avoiding
nested eithers and moving error handling concerns out of the main body.

Fixup OpenTelemetry module documentation

6b978ed

Set env to disable traceing in specs

Wraps get and set with `cache.get` and `cache.set` spans respectively.
The get span includes the key as an attribute and the set span includes
the key, value, and expiration.

This adds a `MonadTracer` constraint to all caching functions, but that
should be manageable since most apps using `freckle-app` and caching
will have it there for `Freckle.App.Database.runDB` anyway.
This only adds topic name to the attributes, since how to deal with
multiple values and a keying function would be somewhat arbitrary. We'll
leave out that behavior for now and possibly add something later when we
start to use this and decide what we'd want.
This adds an outer span, and inner spans for decoding and handling the
message.

In order for spans to be correctly marked as error with exception
details, you need to "throw through" the `inSpan` function, so I had to
do that refactor too. This ultimately worked out simpler by avoiding
nested eithers and moving error handling concerns out of the main body.
@pbrisbin pbrisbin changed the title pb/trace caching Add tracing to Memcached and Kafka operations Oct 25, 2023
@pbrisbin pbrisbin marked this pull request as ready for review October 25, 2023 14:25
Copy link
Member

@stackptr stackptr left a comment

Choose a reason for hiding this comment

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

Had to do a little searching to find what inSpan does, since Hoogle does not show things from hs-opentelemetry-sdk, which is not on Stackage. After that context, this all makes sense.

Throwing exceptions through `inSpan` is what causes them to be marked as
errors. Therefore, we should avoid throwing the `KafkaResponseError`
that represents a polling timeout that way.
"cache.set"
clientSpanArguments
{ Trace.attributes =
HashMap.fromList
Copy link
Contributor

Choose a reason for hiding this comment

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

Offtopic/unimportant, but any interest in adding GHC.Exts.fromList to the prelude? Could have spared you an import here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm into it. I've been trying to migrate SomeModule.toList to the (already prelude-available) one from Data.Foldable as a similar simplification.

@pbrisbin pbrisbin merged commit c5a440a into main Nov 1, 2023
7 checks passed
@pbrisbin pbrisbin deleted the pb/trace-caching branch November 1, 2023 20:22
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