feat: Trace web hook calls

[martinei]

This Pull request attempt to improve tracing in two places:

• Tracing Middleware was not register in the correct order in daemon/serve.go. That led to many DB calls being orphaned because their parent span did not exist. Now most DB calls are under a correct parent span. (I noticed further orphaned DB call spans, but the other root cause is in pop: "Create" does not pass Context properly gobuffalo/pop#685)
• The http client for webhooks is now instrumented as well, so that webhooks are traced

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

This PR still creates the Http client in webhooks. I am not sure if it would be a good Idea to move it to the registry.
Further the client has no timeouts configured. Making the configurable could be a task for further improvements.

I had some trouble because the the config and thus the tracer are "contextualized" - at least that is what the function signatures say. In reality I believe the context param is never used and I think it makes the overall thingh to complex.

[martinei]

I am not sure about the failing tests. As far as I can see those tests are also red on master

[aeneasr]

Those are flaky :) Should be fine!

[aeneasr]

Great stuff! Just a minor remark regarding tests :)

[aeneasr]

Is there some way to write a test for this functionality? It looks fine but we need to ensure:

1. It works
2. It doesn't break when someone changes something

[aeneasr]

Could you move this to ory/x/httpx? It could be an option like

https://github.com/ory/x/blob/e38bb388d3f8982c170d256247a3b4f42a8ce589/httpx/resilient_client.go#L40

so for example:

httpx.ResilientClientWithTracer()

that would allow this to be used across the ecosystem :)

[martinei]

I will have a look, also at the test

[aeneasr]

ory/x is merged :)

Probably makes sense to redo this PR completely from master and just add tracing here:

kratos/driver/registry_default.go

Lines 687 to 698 in d9c8217

	func (m *RegistryDefault) HTTPClient(ctx context.Context) *retryablehttp.Client {
	opts := []httpx.ResilientOptions{
	httpx.ResilientClientWithLogger(m.Logger()),
	httpx.ResilientClientWithMaxRetry(2),
	httpx.ResilientClientWithConnectionTimeout(30 * time.Second),
	}
	if m.Config(ctx).ClientHTTPNoPrivateIPRanges() {
	opts = append(opts, httpx.ResilientClientDisallowInternalIPs())
	}
	return httpx.NewResilientClient(opts...)
	}

[martinei]

Ups. Updating ory/x did not go well. There appear to be breaking changes in the upgraded jsonschema/v3

[aeneasr]

Oh yeah, if you make your changes against #2166 then you won't have any problems upgrading ory/x :)

[Benehiko]

Looks good! I just think it is missing some tests to verify that the changes do fix the problem. Other than that it's good :)

[aeneasr]

Thank you for the review @Benehiko :) @martinei added tests in the PR in ory/x. Given that we only do some bootstrapping here it will be difficult to test this e2e with Jaeger, so I think we can take a leap of faith on this one!

There are however some CI failures, I'm not sure if they're flakes or not (rerunning CI now) but could you (@martinei ) please take a look if they fail?

[martinei]

Thanks @aeneasr This is based on the v0.8 branch which also has failing tests. I was hopping to first see green test there, so I can sort out if this PR brakes anything new.

[aeneasr]

Ah, got it, fair point! Thank you :)