-
Notifications
You must be signed in to change notification settings - Fork 74
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
New telemetry isn't user friendly #129
Comments
The solution to problem number 1 is to not have the telemetry event be tied to the cache. You should have the telemetry event be tied to a deterministic, static Nebulex name and send which cache it came from as part of the event data. This: :ok =
:telemetry.attach(
"nebulex-users-start",
[:myapp, :caches, :users_cache, :command, :start],
&myapp.NebulexTelemetry.handle_event/4,
nil
) Should become this: :ok =
:telemetry.attach(
"nebulex-users-start",
[:myapp, :caches, :command, :start],
&myapp.NebulexTelemetry.handle_event/4,
nil
) And then the cache the telemetry event belongs to would be passed into |
in the vein of user friendliness, some documentation feedback:
and +1 for changing the telemetry event names. |
@SophisticaSean there are several things here. First of all about:
Have you checked the option
The events are at the cache level, you can get the events for each cache you are using, and the cache you know always what adapted is attached to. For a cache using a partitioned or replicated, you can get the events for the cache itself as well as the primary cached used underneath. And for the multi-level cache, you can get the events for each of the levels you're using. For example, if you have a partitioned cache with a telemetry prefix set to
So, I agree that there is a lack of documentation in the adapters about this, as well as more examples with Telemetry, are missing, especially for the distributed/multi-level topologies, but the ability to get the events for each cache being used is there, all built-in adapters as well as the Redis and Cachex one support and emit these events.
I understand, but this is for me something more specific, besides, it is something that can be easily implemented on top of Nebulex if you need it. Anyhow, let's keep this feature request open to discussion, but it should be a separate issue.
I actually gave you some feedback, if you check the PR, you will see I replied to your PR, and dropped several comments, and then I sent you also several update messages about the implementation I was working on at that time. Anyway, I think what you what is something like Spandex but for Nebulex, is that right? If so, one of the features I'd like to provide is something like spandex_ecto but for Nebulex, implementing |
Hey @mjezew !!
You're right, that documentation is missing. I will work on it!
This is actually explained in the docs:
I rather also strict types, but in this case is a bit tricky, because each adapter may have or add more attributes, then if we want to keep it flexible enough, we will end up having a field to the extra attributes being a map anyways, don't you think? – If you check the adapters, the
I think this can be achieved with the |
Awesome. thanks for the heads up on I think something similar to
Would love to see the adapter, as |
Hey @SophisticaSean 👋 ! Sorry for the late response, I took some time off but I'm back now 😄 ! About adding Telemetry.Metrics.summary(
"my_app.cache.command.stop.duration",
unit: {:native, :millisecond},
tags: [:cache, :adapter, :function_name],
tag_values: &Map.merge(&1, %{cache: &1.adapter_meta.cache, adapter: &1.adapter_meta.cache.__adapter__})
) Let me know if that works for you. On the other hand, about |
I've pushed some improvements for the Telemetry documentation on the adapters (4b10c5a). Basically, I added some docs to the adapters explaining better the primary cache events, also added in the Telemetry guide the example of how to include the adapter into the metrics tags, etc. |
Closing this issue since I haven't received any feedback a long time ago, feel free to reopen it or create a new one for reporting related issues or maybe new suggestions. |
Comparing my fork here: https://github.com/SophisticaSean/nebulex/tree/sophisticasean/support-before-after-blocks-in-decorator
In that fork, it was easy to attach to all cache events in your elixir app like such:
lib/myapp/application.ex
Now, with the changes in master, I have to setup telemetry for every cache, 3 or 4 times if I want to include init event:
lib/myapp/application.ex
I already have 6 caches in this rather large app, which means the telemetry attachment is over 400 lines long and I have to remember to add all the events here anytime I add a new cache.
We're also missing events entirely. I want to get telemetry events (and my fork did this) for each adapter so I can plot traces like this:
I'm missing which adapter the telemetry event is coming from. I'm also missing which macro we're using. It would be nice to see if its
cacheable
orcache_put
forNebulex.Caching
module.I'm happy to make these changes, but frankly you didn't provide feedback on my last MR that had all this data and had a better external API. So I'm a bit frustrated as I love this library but I would prefer you help me help you rather than taking a stab at the functionality you think I want.
The text was updated successfully, but these errors were encountered: