Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add instrumentation to content-app #4745

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Add instrumentation to content-app #4745

merged 1 commit into from
Nov 28, 2023

Conversation

lubosmj
Copy link
Member

@lubosmj lubosmj commented Nov 20, 2023

The instrumentation code from the upstream PR was brought down to pulpcore because of pypi/support#3353.

closes #3829

@lubosmj lubosmj changed the title Intrument content-app Instrument content-app Nov 20, 2023
@lubosmj lubosmj changed the title Instrument content-app Add instrumentation to content-app Nov 20, 2023
@lubosmj lubosmj force-pushed the otel-content-app-instr-pr branch 8 times, most recently from 7c1508f to 4411a6f Compare November 21, 2023 10:12
@lubosmj lubosmj marked this pull request as ready for review November 21, 2023 10:21
@lubosmj
Copy link
Member Author

lubosmj commented Nov 21, 2023

I leaned to option B -- temporarily using the code from the upstream repository.

Since no content-app endpoints are designed to handle POST/PUT requests, I am testing only HEAD and GET requests on a file distribution. Do you find these tests satisfactory, @decko?

@lubosmj lubosmj force-pushed the otel-content-app-instr-pr branch 2 times, most recently from af2197f to 65e1b08 Compare November 21, 2023 10:45
@lubosmj
Copy link
Member Author

lubosmj commented Nov 21, 2023

Telemetry tests are enabled only for the Azure runner:

pulpcore/template_config.yml

Line 52 in f0569f4

pulp_env_azure:

@lubosmj lubosmj force-pushed the otel-content-app-instr-pr branch 2 times, most recently from d5fe356 to 585bb08 Compare November 21, 2023 11:51
@lubosmj lubosmj marked this pull request as draft November 21, 2023 12:07
@lubosmj lubosmj force-pushed the otel-content-app-instr-pr branch 6 times, most recently from 90628ae to 42a399b Compare November 21, 2023 15:31
lubosmj
lubosmj commented Nov 21, 2023
View reviewed changes
pulpcore/tests/functional/api/pulp_file/test_telemetry_collection.py Outdated
assert received_otel_span(
{
"http.method": "GET",
"http.target": f"/pulp/content/{distribution.base_path}/{file_name}",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to take care of domains and redirects to content storage here...

@lubosmj lubosmj force-pushed the otel-content-app-instr-pr branch 2 times, most recently from 8de03eb to 72809f3 Compare November 22, 2023 10:52
@lubosmj lubosmj marked this pull request as ready for review November 22, 2023 11:06
decko
decko reviewed Nov 24, 2023
View reviewed changes
pulpcore/tests/functional/assets/otel_server.py Outdated
@@ -68,6 +68,7 @@ async def _traces_handler(request):
for item in span.attributes
}
spans.append(attrs)
print(attrs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was intentional for this to be here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not cluttering the CI logs and it helps with further debugging to see whether the span was received on the server or not. Do you think logging is better than printing? I am going to replace it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

decko
decko approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@decko decko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with just a question

@bmbouter
Copy link
Member

It seems the upstream is not yet able to release this, but they are working on it, see this and this.

Given that it's not available yet, I'm in favor of vendoring it for a release or two in Pulp because we have users who have been waiting and are very eager to try it. Also it comes with functional tests so unlike the previous pull request this does have tests that cover the vendored code.

@mdellweg wdyt about this approach? I don't want to have this merged without your input.

bmbouter
bmbouter approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests look good, thanks for them! Also +1 to @decko 's question about the print() statement. Personally I'd remove it, but it's up to @lubosmj

thanks @lubosmj !

@lubosmj
Add instrumentation to content-app
1332cff 
The instrumentation code from the upstream PR was brought down to pulpcore
because of pypi/support#3353.

closes pulp#3829
@lubosmj lubosmj merged commit 763010e into pulp:main Nov 28, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@decko decko decko approved these changes

@bmbouter bmbouter bmbouter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Open Telemetry support to the pulp-content app
3 participants
@lubosmj @bmbouter @decko