[telemetry] Refactor track_property()
#685
Conversation
vicroms
force-pushed
the
telemetry-refactor
branch
from
c707351
to
caf08c7
Compare
vicroms
force-pushed
the
telemetry-refactor
branch
from
ed9abe8
to
2e783ed
Compare
vicroms
force-pushed
the
telemetry-refactor
branch
from
2d31fb5
to
a5316e5
Compare
|
|
||
| enum class DefineMetric | ||
| { | ||
| AssetSource, |
There was a problem hiding this comment.
Are these referred to more than once? If not, I think the previous form was actually better since we didn't have to decode the enum to a string and back again.
If you keep this, we probably should have a comment about how adding new entries needs to be classified vis a vis GDPR and all that stuff?
| View<MetricEntry<DefineMetric>> Metrics::get_define_metrics() | ||
| { | ||
| static constexpr std::array<MetricEntry<DefineMetric>, static_cast<size_t>(DefineMetric::_COUNT)> ENTRIES{{ | ||
| {DefineMetric::AssetSource, "asset-source"}, |
There was a problem hiding this comment.
An example of why I slightly preferred the string literal form: forgetting an entry in here seems a likely product bug?
There was a problem hiding this comment.
It is intentional that the build fails if an enum is not defined in this array. However, this only checks for length, so one could put a repeated enum in the array to make it pass. This needs some supporting tests.
vicroms
force-pushed
the
telemetry-refactor
branch
from
04f2fc5
to
c2e0f50
Compare
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
|
The overrides are gone. I'll merge this once CI is green since I have a few PRs that depend on this. |
Changes
track_property()to use enums instead of raw strings to collect metrics.The purpose is to provide some basic "type-safety" and make all metrics easily auditable in the code.