-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 time units for interval
probe
#1377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Two things:
- Please add some semantic analysis tests for the new behavior (and old if it doesn't already exist). in tests/semantic_analyser.cpp
- update changelog
Oh and in general you'll want to clang-format as you go. In other words, no commit at the end that does the formatting. |
What is the use case? interval probes are intended for producing output, and I deliberately left off smaller units so that end users did not shoot themselves in the foot. What is the use case for intervals smaller than 1ms? profile probes, on the other hand, are for sampling and can go as fast as possible. |
One concrete use case is a "flight recorder". When $X event occurs, give me the last 500us of data. This would necessitate clearing the map every 500us. You can't really do that with More abstractly, I think it's useful to give the users more flexibility. I don't really see it as a footgun -- if the user actually tries to print something every 1us then events will simply be dropped and the user will be notified of the dropped events. And the final reason would be for consistency. I've received in person feedback in the past about why |
I think it should be doable to build some "footgun" detection into the semantic analyser, like unconditional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@danobi do you think it would be worth it to add some runtime tests here?
Meh, might be more trouble than it's worth. Cuz the test framework doesn't do multi line matches IIRC and then you'd need to print stuff on a single line and that might get flakey with output buffering. |
This patch adds support for new time units
us
andhz
for probeinterval
.Such changes make the time units of
interval
consistent with those ofprofile
. To address #issue1335Checklist
docs/reference_guide.md
CHANGELOG.md