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 #19017 - prefer request id over session id in logging #4410

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Mar 24, 2017

suggested to use session id if available. Although it seemed as a good
idea at the beginning, it has several disadvanteges:

  • it treats UI and API requests differently

  • it makes it impossible to differentiate requests made from the session
    at the same time (notifications is one example, but in general, we
    should not assume the user is performing
    on request at a time)

  • the usability of this as correlation id is questionable, especially
    with asynchronous actions in place

@mention-bot
Copy link

@iNecas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ehelms and @domcleal to be potential reviewers.

#ObjectSpace.dump_all(output: io);
#io.close
puts "============= cleaning…"
GC.start(full_mark: true, immediate_sweep: true);

Choose a reason for hiding this comment

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

Do not use semicolons to terminate expressions.

trap('USR1') do
## beware private temps on RHEL 7+
#io = File.open("/tmp/foreman-dump.json", "w")
#ObjectSpace.dump_all(output: io);

Choose a reason for hiding this comment

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

Trailing whitespace detected.

suggested to use session id if available. Although it seemed as a good
idea at the beginning, it has several disadvanteges:

* it treats UI and API requests differently

* it makes it impossible to differentiate requests made from the session
  at the same time (notifications is one example, but in general, we
  should not assume the user is performing
  on request at a time)

* the usability of this as correlation id is questionable, especially
  with asynchronous actions in place
@iNecas
Copy link
Member Author

iNecas commented Mar 24, 2017

@domcleal @ohadlevy @lzap as people that came to conclusion that session id is better for tagged logging (discussed in #3678), I would like to re-open this as IMO it limits the usability of the feature as mentioned before, without apparent advantage

@lzap
Copy link
Member

lzap commented Mar 27, 2017

Honestly, we should have both. In structured logging you don't have any limitation of what's "too long" because you introduce new field. Both make perfect sense. If I had to pick one (until we implement journald support where we can really send both) I would pick request which is more globally appropriate IMHO. This depends really, I think if you are more into UI troubleshooting you pick session, if into backend (myself included) you pick request.

My biggest argument for request would be that session is unusable for backend while it is still quite usable for UI. Therefore IMHO this is RTM, unless folks have a different opinion (e.g. include both).

Edited: Lack of coffee.

@lzap lzap merged commit 4399420 into theforeman:develop Mar 27, 2017
@iNecas iNecas deleted the log-request-id branch March 27, 2017 15:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants