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

[Reporting] Add context to logging about Space ID handling #80106

Merged
merged 10 commits into from
Oct 13, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Oct 9, 2020

Summary

Problem: after some recent work to bring in better support for Spaces in Reporting, some logging entries are overly-basic due to not having a context of logging tags. To put it more clearly, the logging objects passed through different modules of reporting code carry a "context" of tags to help trace what is happening in the code, using more info than just the log message.

For example, the loggers used during job execution should always have the exportType and jobID as part of the context.

Having this context stay clear at all times will be especially important if Reporting gains an ability to run multiple concurrent jobs.

This PR restores the standard of logging with full context in the Reporting code.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@tsullivan tsullivan requested review from joelgriffith and a team October 12, 2020 20:07
@tsullivan tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppArch v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 12, 2020
@tsullivan tsullivan marked this pull request as ready for review October 12, 2020 20:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

}
}
}

public getFakeRequest(baseRequest: object, spaceId?: string) {
public getFakeRequest(baseRequest: object, spaceId: string | undefined, logger = this.logger) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though a logger is always passed in, I wanted to allow this.logger to be the default since this will now be the only reference to the logger member of the ReportingCore class. If there were no longer references to it, then it should not be declared. However - that process would create churn since we would want to add the logger back in as a member of the class when we need it while registering the task types, in an upcoming PR.

So, it might not seem important to keep the private logger class member, but it helps create a churn of subtractions and additions between different PRs in the code

return spaceId;
} else {
this.logger.info(`Request uses default Space`);
logger.debug(`Request uses default Space`);
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 find it better for this recently added log line to be debug rather than info.

@tsullivan tsullivan changed the title [Reporting] Logger Fixes [Reporting] Add context to logging about Space ID handling Oct 12, 2020
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Small nitpicks with the logging backticks

x-pack/plugins/reporting/server/core.ts Outdated Show resolved Hide resolved
x-pack/plugins/reporting/server/core.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan tsullivan merged commit f09aefb into elastic:master Oct 13, 2020
@tsullivan tsullivan deleted the reporting/logging-context-fixes branch October 13, 2020 23:40
@tsullivan tsullivan added v7.11.0 and removed v7.10.0 labels Oct 14, 2020
tsullivan added a commit to tsullivan/kibana that referenced this pull request Oct 14, 2020
…0106)

* [Reporting] Logger Fixes

* info level to debug level for default space message

* fix context for custom logo

* fix tags

* Update x-pack/plugins/reporting/server/core.ts

Co-authored-by: Joel Griffith <[email protected]>

* rm whitespace

* one more backtick string

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Joel Griffith <[email protected]>
# Conflicts:
#	x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts
tsullivan added a commit that referenced this pull request Oct 15, 2020
…80602)

* [Reporting] Logger Fixes

* info level to debug level for default space message

* fix context for custom logo

* fix tags

* Update x-pack/plugins/reporting/server/core.ts

Co-authored-by: Joel Griffith <[email protected]>

* rm whitespace

* one more backtick string

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Joel Griffith <[email protected]>
# Conflicts:
#	x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants