Adds metrics and tracing to the pulp-content app

[decko]

Closes #3829

[bmbouter]

Can a feature be filed for this and a user facing release note added associated with that feature also please? Thanks!

[bmbouter]

This looks great to me! I agree with vendoring this given the upstream timeline does not seem on the order of days or maybe even weeks.

[ggainey]

Looks good, I'm good w/ us carrying until it appears in an upstream release.

[ggainey]

👍

[mdellweg]

Let's be honest. Instead of calling this an effective copy of a PR (that may or may not be accepted in the current state), let's own this code, make it officialy a part of Pulp and talk about upstreaming it from there.

[decko]

I don't think it's possible @mdellweg since we're only part of the authors of that code.

[dralley]

Make sure that if the license requires a copyright / license notice in copies of the code, that we're complying with that.

[mdellweg]

I should probably elaborate: With owning i don't mean claiming the authorship, but taking the responsibility.

[bmbouter]

re licensing: it's unmerged upstream so I think it's actually not even licensed. When it is merged the upstream will be apache V2.

re owning it: we are owning it since we're contributing it upstream and the vendored version here. The thing I'm trying to avoid is to have it adhere to the standards of the Pulp community when it's actual destination is a contribution to the open-telemetry community which has different standards, norms, and expectations. If there's an issue though, I expect we'll fix that both here and in the upstream for example.

[mdellweg]

re licensing: It's easy then. The original contribution, which we adopt here, was made under the Apache V2 license. So it is that one.

[ipanova]

Apologies for bringing in last minutes changes requests, but would it be possible to add docs? This is an important feature pulp is adding and users will not see it a word about it except for this one liner changelog.
Can at least a docs paragraph be added? In a similar fashion we have mentions about analytics https://github.com/pulp/pulpcore/blob/main/docs/components.rst#analytics-collection

[mdellweg]

Can this work be tested in any reasonable way?

[decko]

Can this work be tested in any reasonable way?

If you're talking about the instrumentation code we have it on the upstream package. I couldn't see a reason to vendor the tests too.
If you're talking about our instance of the aiohttp-server I've found no tests for the authentication middleware or any kind of test that test those pieces together.

Would make sense to have tests for it?

[bmbouter]

Can this work be tested in any reasonable way?

If you're talking about the instrumentation code we have it on the upstream package. I couldn't see a reason to vendor the tests too. If you're talking about our instance of the aiohttp-server I've found no tests for the authentication middleware or any kind of test that test those pieces together.

Would make sense to have tests for it?

I agree with this. This isn't really Pulp code, it's just being included here so we can start using it because the upstream is very slow to respond. Given that the only actual Pulp part is the part enabling it. We'd add tests here just to then remove them when we remove it in favor of the upstream release also.

[decko]

Apologies for bringing in last minutes changes requests, but would it be possible to add docs? This is an important feature pulp is adding and users will not see it a word about it except for this one liner changelog. Can at least a docs paragraph be added? In a similar fashion we have mentions about analytics https://github.com/pulp/pulpcore/blob/main/docs/components.rst#analytics-collection

I "drafted" a first version of it @ipanova. Can you check and left more suggestions?

[bmbouter]

I think this needs to become it's own paragraph expanding on it. What this actually does is run the workers with a different prefix so these docs should give an example of running pulp-api and pulp-content with and without open telemetry support. Then after explaining that we can state that the pulp-oci-images automatically handles that if the PULP_OTEL_ENABLED is set.

[ipanova]

@bmbouter is it pulp-oci-images that handles this automatically? that means not only single container but also operator should be well set here too as well.

[mdellweg]

Can this work be tested in any reasonable way?

If you're talking about the instrumentation code we have it on the upstream package. I couldn't see a reason to vendor the tests too. If you're talking about our instance of the aiohttp-server I've found no tests for the authentication middleware or any kind of test that test those pieces together.
Would make sense to have tests for it?

I agree with this. This isn't really Pulp code, it's just being included here so we can start using it because the upstream is very slow to respond. Given that the only actual Pulp part is the part enabling it. We'd add tests here just to then remove them when we remove it in favor of the upstream release also.

Well, it is surely not upstream code. Hence my ask to really own it here. If we do not make it Pulp code, it's nobody's code. Whatever may or may not be merged upstream eventually, nobody can say. And what is not tested, must be considered broken. Adding all of this to pulp is supposed to generate some side effects. If those are not observable, it's not a feature. Maybe for clarification, I am talking about pulp tests, not unittests for the otel library. However, the latter should also come with the code.

edit: Re unittests: I shifted my mind. Yes, unittest should absolutely come and go with this piece of code, which we also just add to remove it later.

[ipanova]

added. keep it past simple

[ipanova]

@bmbouter is it pulp-oci-images that handles this automatically? that means not only single container but also operator should be well set here too as well.

[bmbouter]

@ipanova yes it's pulp-oci-images that does it with this. I also expect that to work correctly for the operator too.

[bmbouter]

Given that there is not an agreement on the testing requirements for this code, I think we should split the docs into their own PR.

This PR was designed to save time for the upstream to accept and release the work. I believe if we need to write tests also here it will take more time than it saves. We can just let the upstream contribution process work it out and close this PR.

[decko]

Given that there is not an agreement on the testing requirements for this code, I think we should split the docs into their own PR.

This PR was designed to save time for the upstream to accept and release the work. I believe if we need to write tests also here it will take more time than it saves. We can just let the upstream contribution process work it out and close this PR.

I'm closing this PR until further decision about how we should deal with this kind of situation.
And I'm opening another PR just for the documentation.

[mdellweg]

If you're talking about our instance of the aiohttp-server I've found no tests for the authentication middleware or any kind of test that test those pieces together.

We do test the authentication middleware, because it's purpose is to set the user attribute on the request object. We test the domain middleware, because without the domain set, pulp would not work properly. So I read that the otel middleware is supposed to generate some data that is to be collected somewhere. Why is it so hard to assert that this happens according to some expectations?

This PR was designed to save time for the upstream to accept and release the work. I believe if we need to write tests also here it will take more time than it saves. We can just let the upstream contribution process work it out and close this PR.

If closing this PR was meant to prevent writing any tests at all, I must still say functional test that read like a specification for what kind of collected data we expect are still a requirement in my not so humble opinion.

[bmbouter]

This is a different situation from those examples. In those examples the features provided by dependencies are consumed by Pulp and because of that they are able to be asserted on via the Pulp APIs. In this case this feature is offered from the opentelemetry dependency and consumed directly by the user. There is no Pulp API that we can actually make an API assertion on.

That's "why its so hard". There isn't a Pulp API that we can call to intercept this data. We'd be spending time and effort adding this capability to the CI and then assertions to a component other than Pulp. Additionally, consider that we are only temporarily carrying this code until it's available upstream. That info together means we'd be doing a lot of work just to throw it away, and that isn't something we can afford to do.

My ask is the same as it was. Think over my claim that this is not "pulp code". Allow this code to be temporarily carried as-is, and let the actual test contribution happen upstream. That PR does have tests. This is the #1 request from a large upstream user and I'd like us to let them test it prior to the upstream accept + release timeline.

[mdellweg]

@bmbouter
This change is Pulp code and it deserves a good set of functional tests regardless. How else are we ever to know whether it's safe to pick up a new version of all the otel dependencies.

Your arguments only target the question of whether unittests for the unit that is supposed to be removed again are worth the trouble. I'm trying to readjust my request here.

And yes the hard barriers you are talking about for implementing functional tests (it's not accessible from pulp api) are exactly what I expect to be contributed to the pulp set of tests that are supposed to be staying even after bringing the vendored unit upstream.