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

Fixes #26957 - send request id instead session to proxy #6827

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

lzap
Copy link
Member

@lzap lzap commented Jun 5, 2019

We send the data to correlate logs but instead request we send session
id. This is confusing and this patch fixes that. Additionally, it also
now sends both so proxy can actually process and log both as both can be
useful when digging in logs.

Can I ask for this one into 1.22? We are targetting improving logging in
1.22 and this is a tiny bug that can make searching for logs confusing.
@tbrisker @ares

@theforeman-bot
Copy link
Member

Issues: #26957

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

I assume this also needs some change on the proxy side? Too late for 1.22.0, but this is small enough that i'm willing to pull it into 1.22.1 if the other part is also this small.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

test failures related though

@lzap
Copy link
Member Author

lzap commented Jun 5, 2019

No changes needed actually for this to work properly, this core change (which I propose for 1.22) fixed the issue - session id is no longer sent as request id. Foreman proxy processes the parameter correctly (it was just bad data coming from core).

Additionally, it now sends both, this will require change in Foreman Proxy (which I am going to work on later today), however this I don't propose into 1.22. That is improvement and it is not what we should be backporting.

Rebased and fixed the tests.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @lzap !

@tbrisker tbrisker merged commit 48af620 into theforeman:develop Jun 5, 2019
@tbrisker
Copy link
Member

tbrisker commented Jun 5, 2019

1.22.1- ceb8c2a

@lzap lzap deleted the session-id-26957 branch June 5, 2019 13:51
@lzap
Copy link
Member Author

lzap commented Jun 5, 2019

Thanks a bunch! I will demo this tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants