Skip to content

Allow atom :all as :tags option - #126

Closed
hauleth wants to merge 1 commit into
beam-telemetry:mainfrom
hauleth:push-npyrrxvqtrvz
Closed

hauleth wants to merge 1 commit into
beam-telemetry:mainfrom
hauleth:push-npyrrxvqtrvz

Conversation

@hauleth

@hauleth hauleth commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

This is meant for collectors to just use metadata as a whole. The reasoning for that is that in many cases user may know, that all provided metadata will be relevant (especially true for metrics of the current application). Currently the way to extract relevant metrics is to use Map.take/2, but that can introduce unwanted slowdown due to requirement of constructing new map even if all keys are selected.

In my benchmarking of my project Map.take/2 (internal Map.take/3 function to be exact) was responsible for 7.83% of the whole runtime (very tight loop), and I simply take whole metadata into account, so that additional processing is not needed.

@codecov-commenter

codecov-commenter commented Feb 20, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.34%. Comparing base (ab86164) to head (c0a96a4).
⚠️ Report is 12 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #126      +/-   ##
===========================================
- Coverage   100.00%   99.34%   -0.66%     
===========================================
  Files            2        2              
  Lines          145      152       +7     
===========================================
+ Hits           145      151       +6     
- Misses           0        1       +1     
Files with missing lines Coverage Δ
lib/telemetry_metrics.ex 98.98% <100.00%> (-1.02%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd567c2...c0a96a4. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This is meant for collectors to just use metadata as a whole. The
reasoning for that is that in many cases user may know, that all
provided metadata will be relevant (especially true for metrics of the
current application). Currently the way to extract relevant metrics is
to use `Map.take/2`, but that can introduce unwanted slowdown due to
requirement of constructing new map even if all keys are selected.

In my benchmarking of my project `Map.take/2` (internal `Map.take/3`
function to be exact) was responsible for 7.83% of the whole runtime
(very tight loop), and I simply take whole metadata into account, so
that additional processing is not needed.
@josevalim

Copy link
Copy Markdown
Contributor

I wonder if there are other ways to optimize this. The issue with adding :all is that we now need to change all formatters to support it, including the ConsoleFormatter here. So if we are going down this route, I'd rather allow :tags to be a one-arity function, that receives all metadata and returns the metadata we want as tags, this would allow us to deprecate tag_values too.

@josevalim

Copy link
Copy Markdown
Contributor

Actually, I think in many cases projects like Phoenix includes conn and socket as metadata, so using :all will be problematic as it either won't work OR it will cause huge amounts of data to be exported.

@hauleth

hauleth commented Feb 20, 2026

Copy link
Copy Markdown
Contributor Author

@josevalim yeah, but the idea there was to not use that as a default but as an optimisation for places where it really is a problem. That way existing metrics gatherers would still work perfectly fine, it was just for that 1% of the cases where that additional function call does matter.

@josevalim

Copy link
Copy Markdown
Contributor

So let's go with a function and deprecate tag_values, because that would allow you to optimize all cases you care about, since you could also rewrite:

tags: [:foo, :bar]

as:

tags: fn %{foo: foo, bar: bar} -> %{foo: foo, bar: bar} end

WDYT?

@hauleth

hauleth commented Feb 20, 2026

Copy link
Copy Markdown
Contributor Author

That can be a solution as well. The question is whether we should do the translation to function in Telemetry.Metrics or just add new possible value there?

@josevalim

Copy link
Copy Markdown
Contributor

Doing the translation would be a breaking change, so we need add a new value for formatters to handle.

@josevalim

Copy link
Copy Markdown
Contributor

We can do the translation in the future. So if we deprecate tag_values now, we will force formatters to upgrade, and then we translate it in a future release.

hauleth added a commit to hauleth/telemetry_metrics that referenced this pull request Feb 20, 2026
This supersedes `tag_values` function.

Close beam-telemetry#126
hauleth added a commit to hauleth/telemetry_metrics that referenced this pull request Feb 20, 2026
This supersedes `tag_values` function.

Close beam-telemetry#126
hauleth added a commit to hauleth/telemetry_metrics that referenced this pull request Feb 20, 2026
This supersedes `tag_values` function.

Close beam-telemetry#126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants