Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @oncilla)

go/lib/infra/modules/trust/handlers.go, line 70 at r1 (raw file):

	subCtx, cancelF := context.WithTimeout(h.request.Context(), HandlerTimeout)
	defer cancelF()
	subCtx = metrics.CtxWith(subCtx, metrics.TRCReq)

Can you point me out what we do with that, on-line documentation, example? thx!

go/lib/infra/modules/trust/handlers.go, line 343 at r1 (raw file):

// addrLocation extracts the client from an infra request. If the request is nil,
// "app" is returned.
func addrLocation(client net.Addr, localIA addr.IA) string {

Help function about setting labels could be only inside to metrics.
(e.g. not to have long files, polute the go coverage, and sure personal taste)
Also do we want to put comment on the small letters?

go/lib/infra/modules/trust/handlers.go, line 345 at r1 (raw file):

func addrLocation(client net.Addr, localIA addr.IA) string {
	if client == nil {
		return "app"

Usually you do not put direct strings (I do not mind but usually you do ;) )

go/lib/infra/modules/trust/signhelper.go, line 165 at r1 (raw file):

	}
	if v.ignoreSign(cpld, spld.Sign) {
		// Do not increase metric

why? maybe on the comment better

go/lib/infra/modules/trust/trust.go, line 207 at r1 (raw file):

		}
	}
	return store.getTRCFromNetwork(ctx, &trcRequest{

It would have been more intuitive to sto increase the counter when you handle this error.
Otherwise in the future someone might just replicate it.

go/lib/infra/modules/trust/trust.go, line 256 at r1 (raw file):

func (store *Store) insertTRCHookForwarding(ctx context.Context, trcObj *trc.TRC) error {
	if err := store.insertTRCHookLocal(ctx, trcObj); err != nil {
		return err

on purpose we do not put metric here?

go/lib/infra/modules/trust/trust.go, line 388 at r1 (raw file):

		}
	}
	l.Result, chain, err = store.getChainFromNetworkWithMetric(ctx, client, &chainRequest{

suggestion would be that we we not mix the labels with the logic, so we could in the future just remove the metrics part without thinking. For example

chain,err:= store.getChainFromNetwork(ctx... )
l:=getLabels(chain,err)
metrics.Store.Lookup(l).Inc()

go/lib/infra/modules/trust/trust.go, line 414 at r1 (raw file):

		}
		_, err := store.trustdb.InsertChain(ctx, chain)
		if err != nil {

Do we on purpose avoid sending errors?

go/lib/infra/modules/trust/internal/metrics/db.go, line 62 at r1 (raw file):

// Values returns the label values in the order defined by Labels.
func (l *QueryLabels) Values() []string {

no * better

go/lib/infra/modules/trust/internal/metrics/handler.go, line 39 at r1 (raw file):

// Values returns the label values in the order defined by Labels.
func (l *HandlerLabels) Values() []string {

no *

go/lib/infra/modules/trust/internal/metrics/labels_test.go, line 24 at r1 (raw file):

)

func TestQueryLabels(t *testing.T) {

can be an array?

go/lib/infra/modules/trust/internal/metrics/metrics.go, line 33 at r1 (raw file):

	ASInspector     = "as_inspector"
	Load            = "load"
	App             = "application"

I always suggest that unless we need to export, we should keep private

go/lib/infra/modules/trust/internal/metrics/metrics.go, line 54 at r1 (raw file):

	DB = newDB()
	// Handler exposes the handler metrics.
	Handler = newHandler()

nit/ Handler is a name that is overloaded

go/lib/infra/modules/trust/trustdb/metrics.go, line 32 at r1 (raw file):

// WithMetrics wraps the given TrustDB into one that also exports metrics.
func WithMetrics(driver string, trustDB TrustDB) TrustDB {

types / vars should be on top of the file for better reading

go/lib/infra/modules/trust/trustdb/metrics.go, line 41 at r1 (raw file):

	return &metricsTrustDB{
		metricsExecutor: rwDBWrapper,
		db:              trustDB,

trustDB exists twice in the struct, can it be once?

go/lib/infra/modules/trust/trustdb/metrics.go, line 46 at r1 (raw file):

type observer struct {
	driver string

instead of struct/nested struct maybe w could have closure on that.
Also driver i was expected to be something fancier than a string :)

go/lib/infra/modules/trust/trustdb/metrics.go, line 55 at r1 (raw file):

		Driver:    o.driver,
		Operation: op,
		Result:    db.ErrToMetricLabel(action(ctx)),

So basically from the code we do the db action because we want to get a value for the metrics :)
Just was super confused around here

go/lib/infra/modules/trust/trustdb/metrics.go, line 231 at r1 (raw file):

	var res *cert.Chain
	var err error
	db.metrics.Observe(ctx, metrics.GetChainMax, func(ctx context.Context) error {

I am asking for a change but it would be cleaner

res,err:=db.rwDB.GetChainMaxVersion(ctx.ia)
Observe(metrics.GetChainMax,err)
return res,err

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @karampok)

go/lib/infra/modules/trust/handlers.go, line 70 at r1 (raw file):

Done.

go/lib/infra/modules/trust/handlers.go, line 343 at r1 (raw file):

I don't want to put that in the metrics package.
infra pulls in a lot of dependencies, and we do not want that in a metrics package.

go/lib/infra/modules/trust/handlers.go, line 345 at r1 (raw file):

Done.

go/lib/infra/modules/trust/signhelper.go, line 165 at r1 (raw file):

Done.

go/lib/infra/modules/trust/trust.go, line 207 at r1 (raw file):

I don't understand, what counter and what error are you talking about?

go/lib/infra/modules/trust/trust.go, line 256 at r1 (raw file):

yes, there is no counter we could increase here.

go/lib/infra/modules/trust/trust.go, line 388 at r1 (raw file):

I like the idea. 👍
Done

go/lib/infra/modules/trust/trust.go, line 414 at r1 (raw file):

Yes, there is no point of forwarding an error if we cannot insert the data.
There is nothing the receiving end could do about it, nor should it care.

(This is about forwarding, not responding to the peer.)

go/lib/infra/modules/trust/internal/metrics/db.go, line 62 at r1 (raw file):

Done.

go/lib/infra/modules/trust/internal/metrics/handler.go, line 39 at r1 (raw file):

Done.

go/lib/infra/modules/trust/internal/metrics/labels_test.go, line 24 at r1 (raw file):

Done as a map, such that we easily spot which sub test fails.

go/lib/infra/modules/trust/internal/metrics/metrics.go, line 33 at r1 (raw file):

Done.

go/lib/infra/modules/trust/internal/metrics/metrics.go, line 54 at r1 (raw file):

Agreed, but in this case I think it fits pretty well, since those are the Handler metrics.

go/lib/infra/modules/trust/trustdb/metrics.go, line 32 at r1 (raw file):

WithMetrics is ~constructor of metricsTrustDB -> I put it where the constructor would be.

go/lib/infra/modules/trust/trustdb/metrics.go, line 41 at r1 (raw file):

they are different types.

go/lib/infra/modules/trust/trustdb/metrics.go, line 46 at r1 (raw file):

how would that look like?
And what would be the advantage?

go/lib/infra/modules/trust/trustdb/metrics.go, line 55 at r1 (raw file):

made more explicit

go/lib/infra/modules/trust/trustdb/metrics.go, line 231 at r1 (raw file):

that is not possible because the observe also does tracing
-> it starts and stops a span.

Reviewable status: 9 of 18 files reviewed, 2 unresolved discussions (waiting on @karampok and @oncilla)

go/lib/infra/modules/trust/trustdb/metrics.go, line 46 at r1 (raw file):

hard to demonstrate

func WithMetrics(driver string, trustDB TrustDB) TrustDB {
	rwDBWrapper := &metricsExecutor{
		rwDB:              trustDB,
		executeAndObserve: getObserFunc(driver),
	}
	return &metricsTrustDB{
		metricsExecutor: rwDBWrapper,
		db:              trustDB,
	}
}

func getObserFunc(drive string) ( func() ){
   return  func( .....) {
      l:= metrics.QueryLabels{
     ....
     Driver: drive
    ....
     }
  }
  
}

...
db.metricsExecutor.executeAndObserve( ....) instead of  db.metricsExecutor.metrics.Observe

Does that make sense?

In my opinion of course, I think the code could have been simpler, but in this case the overall complexity hides this detail.

Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karampok)

go/lib/infra/modules/trust/trust.go, line 207 at r1 (raw file):

We have metrics increments all in this method to exactly avoid the duplication of increments.
E.g. if you scatter the metrics increase around, you cannot really be sure whether metrics are always increased for every branch in the execution, and it is really hard to check.

With the current approach, you only need to check one single method to see whether metrics are updated for every branch.

go/lib/infra/modules/trust/trustdb/metrics.go, line 46 at r1 (raw file):

tbh, I don't think we should not invest too much time into this package.
it will be removed when we move to the new CPPKI, same for trust/trust.go above.

Reviewable status: complete! all files reviewed, all discussions resolved

Reviewable status: complete! all files reviewed, all discussions resolved

Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

• Shorten database to db.
• Differentiate between requests that could be locally satisfied or not
• change truststore_outgoing_requests_total to truststore_sent_requests_total
• change truststore_served_requests_total to truststore_received_requests_total

Reviewable status: 11 of 18 files reviewed, 1 unresolved discussion (waiting on @karampok and @kormat)

a discussion (no related file):

Done.

Reviewed 7 of 7 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat)

Reviewed 4 of 4 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved