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.

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

Add support for correlation_id to pulp #1017

Merged
merged 1 commit into from Dec 4, 2020
Merged

Conversation

daviddavis
Copy link
Contributor

fixes #4689

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

@pulpbot
Copy link
Member

pulpbot commented Nov 10, 2020

Attached issue: https://pulp.plan.io/issues/4689


Pulp uses ``django-guid`` to append correlation IDs to logging messages. Correlation IDs are
autogenerated by default but can also be sent as a header with each request. They are also
returned as a header in the response and are recorded inthe ``logging_cid`` field of tasks.
Copy link
Member

Choose a reason for hiding this comment

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

s/inthe/in the

cls.task_api = TasksApi(cls.client)

def test_correlation_id(self):
"""Test that a correlation can passed as a header and logged."""
Copy link
Member

Choose a reason for hiding this comment

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

s/can/can be

Copy link
Member

@ipanova ipanova left a comment

Choose a reason for hiding this comment

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

@daviddavis the pr looks good to me however i have not manually tested it

"loggers": {
"": {
# The root logger
"handlers": ["console"],
"level": "INFO",
"filters": ["correlation_id"],
Copy link
Member

Choose a reason for hiding this comment

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

Dec 04 16:01:39 pulp3-source-fedora32.fluffy.example.com gunicorn[44116]: pulp [None]: django_guid:INFO: Header Correlation-ID was not found in the incoming request. Generated new GUID: b9c98a1029fd4dca8be4b5725dc5236e

was it intentional to log this at the info level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately yes. I'm not thrilled by it but it's what django-guid does.

Copy link
Member

Choose a reason for hiding this comment

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

oic

Copy link
Member

Choose a reason for hiding this comment

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

It also logs the same if I provide a correlation ID in the wrong format (not UUID). I think it's very confusing. Maybe we should file an issue/rfe about all that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow. When I submit an invalid uuid, I get:

pulp [None]: django_guid:INFO: Correlation-ID found in the header: 123
pulp [None]: django_guid:WARNING: Failed to validate GUID 123
pulp [None]: django_guid:INFO: 123 is not a valid GUID. New GUID is 487f17535a9a46f3a38dc85b9982f929

What's confusing?

Copy link
Member

@goosemania goosemania Dec 4, 2020

Choose a reason for hiding this comment

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

For http :/pulp/api/v3/repositories/rpm/rpm/ Corellation-ID:tadaaaaaa or POST to this, I get:
pulp [None]: django_guid:INFO: Header `Correlation-ID` was not found in the incoming request. Generated new GUID: 306940d3dad043c78992f3ac70e98ab2

I wonder if it's because it's a synchronous call and not with a task.

Copy link
Member

Choose a reason for hiding this comment

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

/me can't spell Correlation correctly. Problem solved.

Copy link
Member

@goosemania goosemania left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good.

I tested sync requests and tasks, passing custom correlation id and not.
Works as expected if you are passing uuid :) as a correlation id.

@daviddavis
Copy link
Contributor Author

Works as expected if you are passing uuid :) as a correlation id.

FWIW, there is a setting VALIDATE_GUID that can disable this.

@daviddavis daviddavis merged commit ca7dbe1 into pulp:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants