Emit native histograms only when OM 2.0.0 is requested #1128
Conversation
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
| exemplarstr += nh_exemplarstr | ||
| | ||
| # Skip native histogram samples entirely if version < 2.0.0 | ||
| if s.native_histogram and Version(version) < Version('2.0.0'): |
There was a problem hiding this comment.
Couldn't we do this skip on line 95 and then not have to add and Version(version) >= Version('2.0.0') in other places?
There was a problem hiding this comment.
Agreed, if we're skipping anyway I think we could just do one check and the continue logic at the very beginning of native histogram processing. Otherwise if there are multiple entry points we want to check I think it would be nice to have a single variable like native_histograms_supported that we calculate based off the version at the top of the function, then we can add more flags for other open metrics 2.0 things like the inline created timestamp.
| | ||
| value = '' | ||
| if s.native_histogram: | ||
| if s.native_histogram and Version(version) >= Version('2.0.0'): |
There was a problem hiding this comment.
| if s.native_histogram and Version(version) >= Version('2.0.0'): | |
| if native_histogram: |
I think that instead of checking for both source and the flag we can just check to see if the constructed native_histogram variable is no longer empty.
| exemplarstr += nh_exemplarstr | ||
| | ||
| # Skip native histogram samples entirely if version < 2.0.0 | ||
| if s.native_histogram and Version(version) < Version('2.0.0'): |
There was a problem hiding this comment.
Agreed, if we're skipping anyway I think we could just do one check and the continue logic at the very beginning of native histogram processing. Otherwise if there are multiple entry points we want to check I think it would be nice to have a single variable like native_histograms_supported that we calculate based off the version at the top of the function, then we can add more flags for other open metrics 2.0 things like the inline created timestamp.
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
| Thanks for your suggestions, simplified logic accordingly! |
3 participants
This PR addresses issue #1122 . In the present implementation, if the OM version negotiated is < 2.0.0, native histogram samples are completely ignored during the exposition.
As agreed with @csmarchbanks, I choose the approach of adding a
versionparameter to thegenerate_latestfunction, which this way can be intended as "generating the latest of a major version".I added some tests; probably the
test_native_histogram_version_comparisonis a bit redundant, but gives an immediate idea of the different outputs depending on the OM version selected.I foresee more tests should be added based on whatever OM v1.0.0 vs v2.0.0 differences/divergent expected behaviours are going to be there.