[maki5]

POC for #3558

Right now I created a metrics container which include reusable metrics in metrics package, this container is used to initialize and to use metrics, also it allows to add custom created(service defined) metrics. This approach is applied right now just to activitylog service so it can be compared with previous approaches for metrics

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 98 complexity

Metric	Results
Complexity	98

View in Codacy

🟢 Coverage 72.43% diff coverage · +0.19% coverage variation

Metric	Results
Coverage variation	✅ +0.19% coverage variation (-1.00%)
Diff coverage	✅ 72.43% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (3ad81e4)	92407	23478	25.41%
Head commit (88cf2fc)	92715 (+308)	23737 (+259)	25.60% (+0.19%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3763)	341	247	72.43%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[pbleser-oc]

Might be personal opinion, but there's a few things where I'm not sure whether that's the preferred API we would want to work with, especially for additional metrics: putting them into a map and accessing them through their name instead of a variable only has drawbacks.

So maybe we would rather use this top-level Metrics struct via embedding instead? (but not sure whether that would still work for the middleware etc...)
Or just a separate struct for the service-specific metrics instead?

[pbleser-oc]

Might be worth adding a comment above this one, along the lines of:

// To avoid having a high cardinality on the HTTP request path, which may include different
// identifiers and query parameters which would result in a very high number of individual
// values for the path label, we are using chi to resolve the path against the pattern that
// it matches in its route configuration, and we are using that as the value for the path
// label, as the cardinality for those will remain quite small, since it uses the placeholders
// for values instead of the actual values, so e.g. something like
// "/resource/child/{id}" instead of "/resource/child/d6b60c20-8711-4921-9cd6-b45a6a5b6ce1"

[pbleser-oc]

We should look into using Prometheus native histograms instead, but that might be a decision with a different issue, since it requires having a certain minimum version of Prometheus (and Grafana if I'm not mistaken) to be able to use them.
We might want to track both at the moment, to remain backwards compatible.

In any case, would you mind adding a TODO here to reflect that? You could just use what I wrote here in this comment, above.

[pbleser-oc]

Created an issue for that, maybe reference/link it in the comment: #3817

[pbleser-oc]

This is making the use of additional metrics quite cumbersome:

• need to access them through their name
• the compiler isn't telling you that it doesn't exist, have to check at runtime
• have to deal with an error if it doesn't exist, every time
• also losing the type information of each Collector, which means it also needs to be cast to its actual type (Gauge, Counter, ...) every time

[pbleser-oc]

I'm not sure this is the best approach in terms of API:

• when an unsupported APIType is passed, we cannot return an error (we could, but that would make its use cumbersome)
• whether, say, HTTP or GRPC metrics should be enabled will be coming from a boolean config setting somewhere, so having a slice of APIType values is more cumbersome to put together, and certainly not a one-liner

Something like this is probably more fitting to how it will be used in services:

func EnableHTTP(bool)
func EnableGRPC(bool)
func EnableEvents(bool)

Because that's also how it will end up being used:

  m.EnableHTTP(config.HTTP.Enabled)

[pbleser-oc]

worth adding a check for duplicates here, to avoid overwriting a metric with the same name, but that then means that an Option must be able to return an error as well when applied

[pbleser-oc]

That's also making things more cumbersome and not compile-safe, because now we have to access those metrics using

if c, err := m.GetMetric("foo"); err != nil {
  return nil, err
} else {
  c.(Gauge).Increment()
}

instead of just being able to do this if we didn't keep them in a map:

m.Foo.Increment()

[pbleser-oc]

what happens when it's another api here, like events?

[pbleser-oc]

this is what I meant in the comments above: this is more code and more cumbersome than just having a method for each that takes a bool

[pbleser-oc]

Now that I see this... Actually, it might even be questionable whether we should take the namespace as a parameter: isn't it always "opencloud" anyways?
Maybe it would make more sense to only take the name of the service as a parameter, and hardwire the namespace to be "opencloud" in the metrics package?