Skip to content

[improve] Split clientAppId() usage for better client request log.#18166

Closed
lifepuzzlefun wants to merge 1 commit intoapache:masterfrom
lifepuzzlefun:log_client_ip_if_cientAppId_null
Closed

[improve] Split clientAppId() usage for better client request log.#18166
lifepuzzlefun wants to merge 1 commit intoapache:masterfrom
lifepuzzlefun:log_client_ip_if_cientAppId_null

Conversation

@lifepuzzlefun
Copy link
Contributor

@lifepuzzlefun lifepuzzlefun commented Oct 23, 2022

Fixes #18165

Motivation

Current request log use clientAppId for tracing client info.

But when authentication is disabled. clientAppId always return null which makes it is difficult for problem tracing.

This patch split caller for log tracing and for client role retrieve.

Modifications

the following usage are change from clientAppId() to clientRole()

  • FunctionBase
  • SchemasResourceBase
  • TenantsBase
  • Functions
  • Worker
  • TopicsBase
  • most unit test

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

This change added tests and can be verified as follows:

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

But when authentication is disabled. clientAppId always return null which makes it is difficult for problem tracing.

This patch split caller for log tracing and for client role retrieve.
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 23, 2022
"clientRole=" + httpRequest.getAttribute(AuthenticationFilter.AuthenticatedRoleAttributeName);
}

public String clientRole() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to change here, only changing the implementation of clientAppId. Then no need to replace the clientRole somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinks for review. i tried before. but after analysis the code, some code path use clientAppId to get authorization clientRole, direct change origin clientAppId may introduce other bug. sorry for change too much code but i think may be we can find a way to better make the change simle.

@lifepuzzlefun lifepuzzlefun deleted the log_client_ip_if_cientAppId_null branch November 1, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] When authentication is disabled. clientAppId always return null

2 participants