Skip to content

Replace runtime codegen with functions defined in Peep.EventHandler#69

Merged
rkallos merged 1 commit intorkallos:mainfrom
hauleth:push-ytrslrywzpvp
Feb 25, 2026
Merged

Replace runtime codegen with functions defined in Peep.EventHandler#69
rkallos merged 1 commit intorkallos:mainfrom
hauleth:push-ytrslrywzpvp

Conversation

@hauleth
Copy link
Copy Markdown
Contributor

@hauleth hauleth commented Feb 24, 2026

This puts all handler code in Peep.EventHandler and also merges metadata during export, not during insertion. This should speedup inserting metrics by avoiding calls to Map functions.

This also adds support for upcoming telemetry change where :tags attribute in Telemetry.Metric can be a function that takes metadata and returns changed metadata (this is meant to deprecate :tag_values callback).

Close #66

@hauleth
Copy link
Copy Markdown
Contributor Author

hauleth commented Feb 24, 2026

This fails as tests haven't been updated yet.

@hauleth
Copy link
Copy Markdown
Contributor Author

hauleth commented Feb 24, 2026

Tests are updated.

Copy link
Copy Markdown
Owner

@rkallos rkallos 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 at a first glance.

I had dreams around extending Peep.Codegen to define functions for each metric so that busier metrics would appear in flame graphs created with perf, but I never got around to implementing it. Perhaps it might come back at some point in the future, or some clever individual will find another approach.

assert match?(%{statsd_state: nil}, :sys.get_state(pid))
end

test "a worker with non-empty global_tags applies to all metrics" do
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Am I right in thinking that this test was removed because Peep.get_metric/3 was not modified to have global tags added?

Since that function is only really used in tests, I have half a mind to replace it with a helper function that asserts many things about metrics with a single call to the 'blessed' Peep.get_all_metrics/1. If I make that change before merging your PR, you should be able to rebase and keep this test around.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is the reason. Instead of this test I have added new in exporters to check if the exported data contains expected values.

@rkallos rkallos merged commit 677c706 into rkallos:main Feb 25, 2026
1 check passed
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.

Question: add global labels only during export

2 participants