Skip to content

[core.logging] Fix calculating payload bytes for arrays and zlib streams.#92100

Merged
lukeelmers merged 9 commits intoelastic:masterfrom
lukeelmers:fix/payload-bytes
Mar 2, 2021
Merged

[core.logging] Fix calculating payload bytes for arrays and zlib streams.#92100
lukeelmers merged 9 commits intoelastic:masterfrom
lukeelmers:fix/payload-bytes

Conversation

@lukeelmers
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers commented Feb 19, 2021 •
edited
Loading

Closes #90457

Summary

This PR fixes 2 bugs related to how we calculate payload bytes for the purposes of http response logging which was introduced in #87939:

  1. Ensures object responses that are arrays get calculated (in [core.logging] Fix issue where logs fail to calculate size of gunzip streams. #90353 we fixed a bug that was breaking Fleet, but forgot to consider array payload calculations)
  2. Ensures that we calculate payload sizes for the zlib streams which are used in Fleet and SIEM.

Example logs with calculated zlib payloads
Screen Shot 2021-02-19 at 4 01 17 PM

Reviewing / Testing

It is recommended to preview the diff with "hide whitespace changes" enabled.

You need to enable http.server.response logs to test this. Here's the config I've been testing with:

logging:
  events: # add this section to test legacy logs and compare
    request: "*"
    response: "*"

  appenders:
    console:
      type: console
      layout:
        type: pattern
        highlight: true
        pattern: "[%date][%level][%logger] %message"

  root:
    appenders: [console, default]
    level: warn

  loggers:
    - name: http.server.response
      appenders: [console]
      level: debug

@lukeelmers lukeelmers added bug Fixes for quality problems that affect the customer experience Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Logging v7.12.0 v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 19, 2021
@lukeelmers lukeelmers requested a review from mshustov February 19, 2021 23:04
@lukeelmers lukeelmers requested a review from a team as a code owner February 19, 2021 23:04
@lukeelmers lukeelmers self-assigned this Feb 19, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Feb 19, 2021

Pinging @elastic/kibana-core (Team:Core)

@lukeelmers
Copy link
Copy Markdown
Contributor Author

lukeelmers commented Feb 21, 2021

@elasticmachine merge upstream

@lukeelmers
Copy link
Copy Markdown
Contributor Author

lukeelmers commented Feb 22, 2021

@elasticmachine merge upstream

@lukeelmers lukeelmers mentioned this pull request Feb 22, 2021
Closed
mshustov
mshustov reviewed Feb 23, 2021
View reviewed changes
mshustov
mshustov reviewed Feb 23, 2021
View reviewed changes
mshustov
mshustov reviewed Feb 23, 2021
View reviewed changes
src/core/server/http/logging/get_payload_size.ts Outdated
return (
!isBoom(res) &&
res.variety === 'stream' &&
res.source === src &&
Copy link
Copy Markdown
Contributor

@mshustov mshustov Feb 23, 2021

Choose a reason for hiding this comment

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

Why we need such a sophisticated check? Maybe duck-typing would be enough?

Copy link
Copy Markdown
Contributor Author

@lukeelmers lukeelmers Feb 23, 2021

Choose a reason for hiding this comment

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

Re-evaluating each of the typeguards, there's actually quite a bit of cleanup we can do -- for example, we're already checking isBoom earlier in the function so we don't need those checks duplicated in the typeguards, and testing reference equality of res.source and src isn't strictly necessary.

I went ahead and simplified each of them to take the source object and the variety string.

The one for zlib streams is still a little more complex as TS will complain if we just try to check variety === 'stream' && obj.bytesWritten, as obj is unknown, hence the typeof and null checks.

@lukeelmers
Copy link
Copy Markdown
Contributor Author

lukeelmers commented Feb 23, 2021

@elasticmachine merge upstream

@lukeelmers lukeelmers requested a review from mshustov February 23, 2021 22:26
@lukeelmers
Copy link
Copy Markdown
Contributor Author

lukeelmers commented Feb 24, 2021

@elasticmachine merge upstream

@restrry This is ready for another look whenever you get the chance.

@lukeelmers
Copy link
Copy Markdown
Contributor Author

lukeelmers commented Mar 2, 2021

@elasticmachine merge upstream

@lukeelmers lukeelmers enabled auto-merge (squash) March 2, 2021 00:58
@lukeelmers
Copy link
Copy Markdown
Contributor Author

lukeelmers commented Mar 2, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 1.6MB 1.5MB -23.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 104.0KB 104.1KB +82.0B
Unknown metric groups

async chunk count

id before after diff
triggersActionsUi 41 42 +1

History

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

cc @lukeelmers

@lukeelmers lukeelmers merged commit 81348b8 into elastic:master Mar 2, 2021
@kibanamachine kibanamachine mentioned this pull request Mar 2, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 2, 2021
@lukeelmers @kibanamachine
[core.logging] Fix calculating payload bytes for arrays and zlib stre…
d7b7ea1 
…ams. (elastic#92100)
@kibanamachine kibanamachine mentioned this pull request Mar 2, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 2, 2021
@lukeelmers @kibanamachine
[core.logging] Fix calculating payload bytes for arrays and zlib stre…
b89e28f 
…ams. (elastic#92100)
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 2, 2021

💚 Backport successful

✅ 7.12 / #93284
✅ 7.x / #93285

Successful backport PRs will be merged automatically after passing CI.

@lukeelmers lukeelmers deleted the fix/payload-bytes branch March 2, 2021 18:40
kibanamachine added a commit that referenced this pull request Mar 2, 2021
@kibanamachine @lukeelmers
[core.logging] Fix calculating payload bytes for arrays and zlib stre…
020437c 
…ams. (#92100) (#93284)

Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
kibanamachine added a commit that referenced this pull request Mar 2, 2021
@kibanamachine @lukeelmers
[core.logging] Fix calculating payload bytes for arrays and zlib stre…
0627307 
…ams. (#92100) (#93285)

Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mshustov mshustov mshustov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukeelmers lukeelmers

Labels

auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:Logging release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.12.0 v7.13.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[core.logging] Request/Response logging cleanup tasks.

5 participants

@lukeelmers @elasticmachine @kibanamachine @mshustov @joshdover