Replace Temporary Views with Metrics View API #7474
Conversation
| HttpMetricsViews.registerViews(builder); | ||
| RpcMetricsView.registerViews(builder); | ||
| return builder; | ||
| }) |
There was a problem hiding this comment.
I think this might be a bad idea. Somewhat surprisingly, doing this prevents a user from using view to drop the instrument or adjust its attributes. This is because when multiple views are registered match a particular instrument, there is no merging of the views. Instead, multiple metrics are produced.
To see what I mean, try configuring the SDK in the test code of this branch to add a view which drops http.server.duration or changes the attributes of http.server.duration. You should multiple metrics named http.server.duration in the logs, each which the configuration of one of the matching views.
There was a problem hiding this comment.
Yes I suspected the same, hence I am still thinking of how to approach this from the user perspective as I marked in the checklist.
But anyhow I believe that making views instead of hard coded filtering would still be the first step in this direction.
The current solution I have is that the user overrides the SdkMetricProvider completely instead of adding in more views over it.
Another approach I believe would be to allow user to customise and configure these views like a decorator pattern by exposing some sort of APIs.
Third approach which I am exploring is to move these configs to view config files and allows user to override these config files either by merging or replacing the config files. This is a more plausible approach but currently due to some dependency issues I am ending up with NoClassDef error.
May be the first draft won't be a cleaner solution for the user to integrate because I suspect that some changes in the base libraries would be needed as well. But it will still be better than having no solution at all like today.
@trask @mateuszrzeszutek do you have some suggestions?
There was a problem hiding this comment.
This would also introduce a breaking change for the library instrumentation users (including spring sleuth/observability): they'd now have to add views by themselves, and ensure that they views match the OTel metrics spec.
I think this should be resolved on the spec level. I've created a separate issue just for this: open-telemetry/opentelemetry-specification#3061
I think this is one of the two known use cases for the future "hint API", and that it's nearly impossible to resolve this (and not break anything) without the necessary API changes.
| HttpMetricsViews.registerViews(builder); | ||
| RpcMetricsView.registerViews(builder); | ||
| return builder; | ||
| }) |
There was a problem hiding this comment.
This would also introduce a breaking change for the library instrumentation users (including spring sleuth/observability): they'd now have to add views by themselves, and ensure that they views match the OTel metrics spec.
I think this should be resolved on the spec level. I've created a separate issue just for this: open-telemetry/opentelemetry-specification#3061
I think this is one of the two known use cases for the future "hint API", and that it's nearly impossible to resolve this (and not break anything) without the necessary API changes.
|
@jack-berg @mateuszrzeszutek I have one more idea for an interim solution. What if the view user wants to customise he drops that view and instead creates a new view. Like Instument = http.client.duration I haven't tried this, but conceptually it should drop the default view and let the user create custom views with attributes they desire. This should not produce 2 views and aggregations and of the same name. |
|
So I realised that this won't work because we can't drop the view created internally. But this lead me thinking what is the point of creating 2 storages in same instrumentation scope with same metric name view. Can we just override the view with the latest registered view that belong to the same scope with the same view name? That should solve all our problems. |
|
hey @anuragagarwal561994 I didn't follow your latest comment, can you explain your latest thoughts in more detail? thx! |
|
closing, this is resolved now by #9440 |
Closes #6819
Closes #6209
Closes #6818