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

The macros no longer support dynamic keys #165

Closed
Marwes opened this issue Jan 30, 2021 · 3 comments · Fixed by #170
Closed

The macros no longer support dynamic keys #165

Marwes opened this issue Jan 30, 2021 · 3 comments · Fixed by #170
Labels
C-macros Component: macros. E-complex Effort: complex. T-enhancement Type: enhancement.

Comments

@Marwes
Copy link

Marwes commented Jan 30, 2021

Why was this removed in 0.13? It is still possible to call the recorder methods manually but constructing the Key is rather annoying. Used dynamic keys for things like:

metrics::counter!(format!("response_status_{}", response.status().as_u16()), 1);
@tobz
Copy link
Member

tobz commented Jan 30, 2021

Well, the simple answer is: practically speaking, we never designed it to be specifically allowed, and it working as such was mostly a side effect of all of the Into<T> impls we had. The example you gave would normally be achieved with something like:

metrics::increment!("http_responses", "status" => response.status().as_u16().as_str());

There's probably a way to bring this back: Key/KeyData definitely support initialization from owned strings. The only current hurdle is the macros themselves, and the arguments they expect. I'll have to think about it.

@tobz tobz added C-macros Component: macros. E-complex Effort: complex. T-enhancement Type: enhancement. labels Feb 1, 2021
@tobz
Copy link
Member

tobz commented Feb 2, 2021

I've been toying with the macro stuff, and this is totally doable. I'm still in the process of going through and adding a lot more unit tests to make sure this works as intended in all cases.

You can follow the PR: #170

@tobz tobz linked a pull request Feb 2, 2021 that will close this issue
@tobz tobz closed this as completed in #170 Feb 3, 2021
@tobz
Copy link
Member

tobz commented Feb 3, 2021

Merged and released. I cut all new minors for essentially all of the crates. Given the new tests that were added, there's fairly solid coverage of the macros, but let me know if anything seems amiss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-macros Component: macros. E-complex Effort: complex. T-enhancement Type: enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants