monotonic sampled counter nanos should remain int64_t #88
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These values are not expected to rollover and will not ever go as high as the actual data values.
copperlight
force-pushed
the
fix-uint-parsing-for-extra
branch
from
cbaf64f to
f2f2aa2
Compare
copperlight
added a commit
to copperlight/spectatord
that referenced
this pull request
Jun 14, 2024
This change fixes a couple of assumptions that were made in Netflix-Skunkworks#87 and Netflix-Skunkworks#88. These changes were made to address a usage issue for monotonic counters in the spectator-go thin client library. For that library, a common use case is to sample uint64 monotonic data sources, so the data type for that meter type in the library was set to uint64. This lead to a need to cascade that change into spectatord. However, changing the existing data type for `C` from `double` to `uint64_t` caused the monotonic counter usage in atlas-system-agent to start partially failing on parsing numbers. When we made these changes, we overlooked the fact that the `double` data type is mostly used when monotonic data sources need to be converted into base units for recording values. So, a monotonic data source recording microsecond values would have to be divided by 1000, and this results in a fractional number, which is not parsed by `strtoull`. Example error message: ``` While parsing sys.pressure.some,xatlas.process=atlas-system-agent,id=io:117.094624: Got 117 parsing value, ignoring chars starting at .094624 ``` The correct way to support all of these use cases, and to preserve backwards compatibility, is to introduce a new meter type `MonotonicCounterUint` which supports the `uint64_t` data type and handles rollovers, while reverting the changes to `MonotonicCounters` (`C`) and `MonotonicSampled` (`X`). In reverting the changes, we kept the original implemenation of disallowing negative value updates, because there is not a use case where that should happen. We chose not to create a `MonotonicSampledUint` at this point in time, because this is an experimental meter type and there is not currently any demand for it. This strategy mirrors the spectator-java implementation: * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounter-T-java.util.function.ToLongFunction- * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounterDouble-T-java.util.function.ToDoubleFunction-
copperlight
added a commit
to copperlight/spectatord
that referenced
this pull request
Jun 14, 2024
This change fixes a couple of assumptions that were made in Netflix-Skunkworks#87 and Netflix-Skunkworks#88. These changes were made to address a usage issue for monotonic counters in the spectator-go thin client library. For that library, a common use case is to sample uint64 monotonic data sources, so the data type for that meter type in the library was set to uint64. This lead to a need to cascade that change into spectatord. However, changing the existing data type for `C` from `double` to `uint64_t` caused the monotonic counter usage in atlas-system-agent to start partially failing on parsing numbers. When we made these changes, we overlooked the fact that the `double` data type is mostly used when monotonic data sources need to be converted into base units for recording values. So, a monotonic data source recording microsecond values would have to be divided by 1000, and this results in a fractional number, which is not parsed by `strtoull`. Example error message: ``` While parsing sys.pressure.some,xatlas.process=atlas-system-agent,id=io:117.094624: Got 117 parsing value, ignoring chars starting at .094624 ``` The correct way to support all of these use cases, and to preserve backwards compatibility, is to introduce a new meter type `MonotonicCounterUint` which supports the `uint64_t` data type and handles rollovers, while reverting the changes to `MonotonicCounters` (`C`) and `MonotonicSampled` (`X`). In reverting the changes, we kept the original implemenation of disallowing negative value updates, because there is not a use case where that should happen. We chose not to create a `MonotonicSampledUint` at this point in time, because this is an experimental meter type and there is not currently any demand for it. This strategy mirrors the spectator-java implementation: * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounter-T-java.util.function.ToLongFunction- * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounterDouble-T-java.util.function.ToDoubleFunction- This change also updates the admin server `/metrics` endpoint, so that the new meter type will be reflected in the output. A `stats` block is also added to the payload to help make it easier to get a sense of the number of meters that are in use.
copperlight
added a commit
to copperlight/spectatord
that referenced
this pull request
Jun 14, 2024
This change fixes a couple of assumptions that were made in Netflix-Skunkworks#87 and Netflix-Skunkworks#88. These changes were made to address a usage issue for monotonic counters in the spectator-go thin client library. For that library, a common use case is to sample uint64 monotonic data sources, so the data type for that meter type in the library was set to uint64. This lead to a need to cascade that change into spectatord. However, changing the existing data type for `C` from `double` to `uint64_t` caused the monotonic counter usage in atlas-system-agent to start partially failing on parsing numbers. When we made these changes, we overlooked the fact that the `double` data type is mostly used when monotonic data sources need to be converted into base units for recording values. So, a monotonic data source recording microsecond values would have to be divided by 1000, and this results in a fractional number, which is not parsed by `strtoull`. Example error message: ``` While parsing sys.pressure.some,xatlas.process=atlas-system-agent,id=io:117.094624: Got 117 parsing value, ignoring chars starting at .094624 ``` The correct way to support all of these use cases, and to preserve backwards compatibility, is to introduce a new meter type `MonotonicCounterUint` which supports the `uint64_t` data type and handles rollovers, while reverting the changes to `MonotonicCounters` (`C`) and `MonotonicSampled` (`X`). In reverting the changes, we kept the original implemenation of disallowing negative value updates, because there is not a use case where that should happen. We chose not to create a `MonotonicSampledUint` at this point in time, because this is an experimental meter type and there is not currently any demand for it. This strategy mirrors the spectator-java implementation: * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounter-T-java.util.function.ToLongFunction- * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounterDouble-T-java.util.function.ToDoubleFunction- This change updates the admin server `/metrics` endpoint, so that the new meter type will be reflected in the output. A `stats` block is also added to the payload to help make it easier to get a sense of the number of meters that are in use.
copperlight
added a commit
to copperlight/spectatord
that referenced
this pull request
Jun 14, 2024
This change fixes a couple of assumptions that were made in Netflix-Skunkworks#87 and Netflix-Skunkworks#88. These changes were made to address a usage issue for monotonic counters in the spectator-go thin client library. For that library, a common use case is to sample uint64 monotonic data sources, so the data type for that meter type in the library was set to uint64. This lead to a need to cascade that change into spectatord. However, changing the existing data type for `C` from `double` to `uint64_t` caused the monotonic counter usage in atlas-system-agent to start partially failing on parsing numbers. When we made these changes, we overlooked the fact that the `double` data type is mostly used when monotonic data sources need to be converted into base units for recording values. So, a monotonic data source recording microsecond values would have to be divided by 1000, and this results in a fractional number, which is not parsed by `strtoull`. Example error message: ``` While parsing sys.pressure.some,xatlas.process=atlas-system-agent,id=io:117.094624: Got 117 parsing value, ignoring chars starting at .094624 ``` The correct way to support all of these use cases, and to preserve backwards compatibility, is to introduce a new meter type `MonotonicCounterUint` which supports the `uint64_t` data type and handles rollovers, while reverting the changes to `MonotonicCounters` (`C`) and `MonotonicSampled` (`X`). In reverting the changes, we kept the original implemenation of disallowing negative value updates, because there is not a use case where that should happen. We chose not to create a `MonotonicSampledUint` at this point in time, because this is an experimental meter type and there is not currently any demand for it. This strategy mirrors the spectator-java implementation: * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounter-T-java.util.function.ToLongFunction- * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounterDouble-T-java.util.function.ToDoubleFunction- This change updates the admin server `/metrics` endpoint, so that the new meter type will be reflected in the output. A `stats` block is also added to the payload to help make it easier to get a sense of the number of meters that are in use.
copperlight
added a commit
that referenced
this pull request
Jun 14, 2024
This change fixes a couple of assumptions that were made in #87 and #88. These changes were made to address a usage issue for monotonic counters in the spectator-go thin client library. For that library, a common use case is to sample uint64 monotonic data sources, so the data type for that meter type in the library was set to uint64. This lead to a need to cascade that change into spectatord. However, changing the existing data type for `C` from `double` to `uint64_t` caused the monotonic counter usage in atlas-system-agent to start partially failing on parsing numbers. When we made these changes, we overlooked the fact that the `double` data type is mostly used when monotonic data sources need to be converted into base units for recording values. So, a monotonic data source recording microsecond values would have to be divided by 1000, and this results in a fractional number, which is not parsed by `strtoull`. Example error message: ``` While parsing sys.pressure.some,xatlas.process=atlas-system-agent,id=io:117.094624: Got 117 parsing value, ignoring chars starting at .094624 ``` The correct way to support all of these use cases, and to preserve backwards compatibility, is to introduce a new meter type `MonotonicCounterUint` which supports the `uint64_t` data type and handles rollovers, while reverting the changes to `MonotonicCounters` (`C`) and `MonotonicSampled` (`X`). In reverting the changes, we kept the original implemenation of disallowing negative value updates, because there is not a use case where that should happen. We chose not to create a `MonotonicSampledUint` at this point in time, because this is an experimental meter type and there is not currently any demand for it. This strategy mirrors the spectator-java implementation: * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounter-T-java.util.function.ToLongFunction- * https://www.javadoc.io/static/com.netflix.spectator/spectator-api/1.7.13/com/netflix/spectator/api/patterns/PolledMeter.Builder.html#monitorMonotonicCounterDouble-T-java.util.function.ToDoubleFunction- This change updates the admin server `/metrics` endpoint, so that the new meter type will be reflected in the output. A `stats` block is also added to the payload to help make it easier to get a sense of the number of meters that are in use.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These values are not expected to rollover and will not ever go as high as the actual data values.