HTTP Metrics Path Matching

[nelson-parente]

Description

This PR implements the proposal dapr/proposals#58.

spec:
  metric:
    enabled: true
    http:
      increasedCardinality: true
      pathMatching:
        ingress:
        - /orders/{orderID}/items/{itemID}
        - /users/{userID}
        egress:
        - /orders
        - /orders/{orderID}/items/{itemID}

PathMatching configuration is opt-in and ensures consistent representation of HTTP request paths in server metrics.

Issue reference

HTTP Metrics Path Matching Proposal: dapr/proposals#58
Dapr Low Cardinality Issue with initial motivation for the proposal: #7719

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Unit tests passing
• End-to-end tests passing
• Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
• Specification has been updated / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
• Provided sample for the feature / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

[ItalyPaleAle]

Normally the mainainters would tell you it's a bit premature to do the work before the proposal is even reviewed...!

[artursouza]

This proposal was reviewed among some runtime maintainers and people got invited to participate in the discussion too. While there is space for discussion, multiple maintainers thought this proposal to be reasonable and low risk to start implementation.

[artursouza]

@nelson-parente Please, address the optimization suggestions as this is a hot path for Dapr and can cause a performance regression.

Thanks, @ItalyPaleAle

[ItalyPaleAle]

This proposal was reviewed among some runtime maintainers and people got invited to participate in the discussion too. While there is space for discussion, multiple maintainers thought this proposal to be reasonable and low risk to start implementation.

This is interesting as every time I was in @nelson-parente's shoes, the response I got from maintainers was very different. I have not seen voting happening (to my understanding, you will need the majority of 3 votes - 1 person for each company), and only 1 person commented.

[yaron2]

The PR is still pending proposal approval

[JoshVanL]

Thanks @nelson-parente, couple more from me 🙂

[JoshVanL]

Rather than have 2 HandlerFunc registrations, we can optionally append "/" if in non-legacy mode and remove dupes in the paths slice.

if !legacy {
  paths = append(paths, "/")
  slices.Sort(paths)
  paths = slices.Compact(paths)
}

Suggested change

	if !catchAllRegistered {
	mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	if !legacy {
	rw, ok := w.(*pathMatchingRW)
	if !ok {
	log.Errorf("Failed to cast to PathMatchingRW")
	return
	}
	rw.MatchedPath = unmatchedPathPlaceholder
	}
	}))
	}

[nelson-parente]

I think we need the 2 handlers registration because we want to distinguish when the user explicitly sets the "/" path or not.

When the user sets the "/" path we match it and use it otherwise if the user does not set a "/" we fallback to the unmatched placeholder "_" so we need to have different handler funcs for those cases.

[nelson-parente]

done! we always need to register the catch all otherwise serveHTTP will throw an error when it doesn't match the path.

[JoshVanL]

Why is this required?

[nelson-parente]

Paths not starting / are invalid and the request will return an error on the handle function.

I noticed this when testing and we had errors with dapr/configpath.

[nelson-parente]

Update: all invoke method paths are coming without the initial slash. I fixed the dapr/config but all these others are using the *invokev1.InvokeMethodRequest and I don't really know the why these don't have the initial slash.