refactor: Use two instances of the query service for queries and https outcall transform functions

[dsarlis]

In a previous change, the older AnonymousQueryService was dropped in order to re-use the same QueryService for handling both regular queries and transform functions from https outcalls.

While this change is a step in a good direction as we don't have mostly duplicate code in AnonymousQueryService, it couples regular queries with transform function executions. To provide better quality of service, go back to using two instances of the same service. This way we can get the best of both worlds: code reusability as well as isolation between the two types of queries.

[dsarlis]

One part where I would appreciate any better ideas is how to handle instantiating the same service twice, specifically because of registering metrics with the same name (if done naively).

For now I took the approach of passing an extra "namespace" argument when creating the service which is used as the prefix of the exported metrics. While it's not too bad, it still feels somewhat hacky. If anyone has a better idea, let me know.

[eichhorl]

One part where I would appreciate any better ideas is how to handle instantiating the same service twice, specifically because of registering metrics with the same name (if done naively).

For now I took the approach of passing an extra "namespace" argument when creating the service which is used as the prefix of the exported metrics. While it's not too bad, it still feels somewhat hacky. If anyone has a better idea, let me know.

I guess one alternative could be to instead switch to a HistogramVec and use the "namespace" as a label. That might make it easier to aggregate over both namespaces if that's of any interest, but I don't have a strong opinion.

[dsarlis]

One part where I would appreciate any better ideas is how to handle instantiating the same service twice, specifically because of registering metrics with the same name (if done naively).
For now I took the approach of passing an extra "namespace" argument when creating the service which is used as the prefix of the exported metrics. While it's not too bad, it still feels somewhat hacky. If anyone has a better idea, let me know.

I guess one alternative could be to instead switch to a HistogramVec and use the "namespace" as a label. That might make it easier to aggregate over both namespaces if that's of any interest, but I don't have a strong opinion.

That's an interesting alternative. It does feel kinda wrong though as each service has a single metric, so making it a HistogramVec to support different namespace sounds like it's still abusing things a bit. Also, I doubt there's gonna be a third service at some point, so the aggregation is still relatively easy, we just need to add the two metrics. So, I think I'll stick to the current approach.

[kpop-dfinity]

One part where I would appreciate any better ideas is how to handle instantiating the same service twice, specifically because of registering metrics with the same name (if done naively).
For now I took the approach of passing an extra "namespace" argument when creating the service which is used as the prefix of the exported metrics. While it's not too bad, it still feels somewhat hacky. If anyone has a better idea, let me know.

I guess one alternative could be to instead switch to a HistogramVec and use the "namespace" as a label. That might make it easier to aggregate over both namespaces if that's of any interest, but I don't have a strong opinion.

That's an interesting alternative. It does feel kinda wrong though as each service has a single metric, so making it a HistogramVec to support different namespace sounds like it's still abusing things a bit. Also, I doubt there's gonna be a third service at some point, so the aggregation is still relatively easy, we just need to add the two metrics. So, I think I'll stick to the current approach.

Another alternative would be to use the Histogram::with_opts constructor and provide a list of fixed labels (see https://sourcegraph.com/github.com/dfinity/ic/-/blob/rs/artifact_pool/src/consensus_pool.rs?L137-147 for an example of how we do it in the consensus pool)

[dsarlis]

One part where I would appreciate any better ideas is how to handle instantiating the same service twice, specifically because of registering metrics with the same name (if done naively).
For now I took the approach of passing an extra "namespace" argument when creating the service which is used as the prefix of the exported metrics. While it's not too bad, it still feels somewhat hacky. If anyone has a better idea, let me know.

I guess one alternative could be to instead switch to a HistogramVec and use the "namespace" as a label. That might make it easier to aggregate over both namespaces if that's of any interest, but I don't have a strong opinion.

That's an interesting alternative. It does feel kinda wrong though as each service has a single metric, so making it a HistogramVec to support different namespace sounds like it's still abusing things a bit. Also, I doubt there's gonna be a third service at some point, so the aggregation is still relatively easy, we just need to add the two metrics. So, I think I'll stick to the current approach.

Another alternative would be to use the Histogram::with_opts constructor and provide a list of fixed labels (see https://sourcegraph.com/github.com/dfinity/ic/-/blob/rs/artifact_pool/src/consensus_pool.rs?L137-147 for an example of how we do it in the consensus pool)

I'm actually not sure any of these would work after thinking about it some more. As long as the metric name is the same, we're still gonna have an issue I think as the metric name cannot be registered twice with the MetricsRegistry we use. Labels do not matter in that respect.

[dsarlis]

One part where I would appreciate any better ideas is how to handle instantiating the same service twice, specifically because of registering metrics with the same name (if done naively).
For now I took the approach of passing an extra "namespace" argument when creating the service which is used as the prefix of the exported metrics. While it's not too bad, it still feels somewhat hacky. If anyone has a better idea, let me know.

I guess one alternative could be to instead switch to a HistogramVec and use the "namespace" as a label. That might make it easier to aggregate over both namespaces if that's of any interest, but I don't have a strong opinion.

That's an interesting alternative. It does feel kinda wrong though as each service has a single metric, so making it a HistogramVec to support different namespace sounds like it's still abusing things a bit. Also, I doubt there's gonna be a third service at some point, so the aggregation is still relatively easy, we just need to add the two metrics. So, I think I'll stick to the current approach.

Another alternative would be to use the Histogram::with_opts constructor and provide a list of fixed labels (see https://sourcegraph.com/github.com/dfinity/ic/-/blob/rs/artifact_pool/src/consensus_pool.rs?L137-147 for an example of how we do it in the consensus pool)

I'm actually not sure any of these would work after thinking about it some more. As long as the metric name is the same, we're still gonna have an issue I think as the metric name cannot be registered twice with the MetricsRegistry we use. Labels do not matter in that respect.

Hmm, seems I was wrong. Trying out @kpop-dfinity's suggestion seems to work. Not entirely sure it's a lot better but I'm also ok with that and it doesn't make a whole lot of difference in the amount of changes needed.