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

Tpv2 logging overhaul #7673

Merged
merged 26 commits into from
Aug 11, 2023
Merged

Conversation

ocket8888
Copy link
Contributor

This PR overhauls the way TPv2 does logging to be more consistent. At present, it's not configurable, it just omits debug-level messages if it's in a production environment. The idea was primarily to add it to the server, which had sparse and opaque logging, but a logging service was also added to the UI to allow for a consistent logging format there. Really all that does is add timestamps to everything (so far).


Which Traffic Control components are affected by this PR?

  • Traffic Portal (experimental v2)

What is the best way to verify this PR?

Make sure the tests pass, look at the server logs and see if the format looks good.

PR submission checklist

  • This PR has tests
  • This PR has documentation
  • This PR doesn't need a CHANGELOG.md entry
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution experimental a feature/component not directly supported by ATC Traffic Portal v2 Related to the experimental Traffic Portal version 2 labels Jul 25, 2023
@ocket8888 ocket8888 marked this pull request as ready for review July 25, 2023 18:59
@ocket8888 ocket8888 force-pushed the tpv2/server-logging-overhaul branch from 69a32a5 to 51e65ff Compare July 26, 2023 14:56
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #7673 (4cef64e) into master (849d166) will increase coverage by 35.82%.
The diff coverage is 76.76%.

@@              Coverage Diff              @@
##             master    #7673       +/-   ##
=============================================
+ Coverage     29.78%   65.60%   +35.82%     
  Complexity       98       98               
=============================================
  Files           802      323      -479     
  Lines         85789    12829    -72960     
  Branches        952      965       +13     
=============================================
- Hits          25550     8417    -17133     
+ Misses        58099     4051    -54048     
+ Partials       2140      361     -1779     
Flag Coverage Δ
golib_unit ?
grove_unit ?
t3c_unit ?
traffic_monitor_unit ?
traffic_ops_unit ?
traffic_portal_v2 74.38% <76.76%> (+0.68%) ⬆️
traffic_stats_unit ?
unit_tests 74.38% <76.76%> (+47.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...al/src/app/shared/interceptor/error.interceptor.ts 3.12% <0.00%> (-0.21%) ⬇️
...tal/traffic-portal/src/app/shared/shared.module.ts 100.00% <ø> (ø)
experimental/traffic-portal/src/app/utils/index.ts 100.00% <ø> (ø)
...src/app/core/cdns/cdn-table/cdn-table.component.ts 35.13% <20.00%> (+0.88%) ⬆️
...servers/server-details/server-details.component.ts 61.53% <27.27%> (+0.27%) ⬆️
...ob-dialog/new-invalidation-job-dialog.component.ts 94.91% <33.33%> (+0.08%) ⬆️
...ies/topology-details/topology-details.component.ts 55.93% <33.33%> (+0.75%) ⬆️
...rc/app/shared/current-user/current-user.service.ts 89.28% <33.33%> (-1.63%) ⬇️
...delivery-service/new-delivery-service.component.ts 73.71% <40.00%> (ø)
...traffic-portal/src/app/api/testing/user.service.ts 50.70% <50.00%> (+0.34%) ⬆️
... and 43 more

... and 483 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shamrickus shamrickus self-assigned this Jul 27, 2023
@ocket8888 ocket8888 force-pushed the tpv2/server-logging-overhaul branch 2 times, most recently from 30de8f1 to 2fd08e4 Compare July 28, 2023 16:40
Copy link
Member

@shamrickus shamrickus left a comment

Choose a reason for hiding this comment

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

E2E tests are failing because the SSR server is hanging up on something. When I run it locally the server says its running but it never handles requests (including proxy requests) and nothing is logged. This happens in both dev and prod mode.

@ocket8888 ocket8888 force-pushed the tpv2/server-logging-overhaul branch from 2fd08e4 to d6dba19 Compare August 2, 2023 20:29
Copy link
Member

@shamrickus shamrickus left a comment

Choose a reason for hiding this comment

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

Confirmed logger works and logging format makes sense, just have some minor comments.

Because they're exported from utils, so the import wouldn't stutter in
most cases even if you decide to import the whole module as a namespace
(for whatever reason).
this simplifies it a bit and also switched to using fs/promises which is
a bit faster
Also removes some try/catch blocks that appeared to be concealing test
failures?
Also alphabetically sorted the declarations for the core module - we
were declaring a few things multiple times.
Makes use of the utils package to avoid the need for 'as' statements
@ocket8888 ocket8888 force-pushed the tpv2/server-logging-overhaul branch from 2e31dbc to 4cef64e Compare August 9, 2023 14:52
Copy link
Member

@shamrickus shamrickus left a comment

Choose a reason for hiding this comment

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

LGTM

@shamrickus shamrickus merged commit 08cd924 into apache:master Aug 11, 2023
9 checks passed
@ocket8888 ocket8888 deleted the tpv2/server-logging-overhaul branch August 11, 2023 18:12
cybertunnel pushed a commit to cybertunnel/trafficcontrol that referenced this pull request Aug 16, 2023
* Add a logging class

* re-name things to include a Log prefix

Because they're exported from utils, so the import wouldn't stutter in
most cases even if you decide to import the whole module as a namespace
(for whatever reason).

* Add middleware for logging and error handling

* Move file compression stuff into middleware

this simplifies it a bit and also switched to using fs/promises which is
a bit faster

* rework TO proxy handler to use a logger

* Add a front-end logging service

* fixup allowed console methods now that there's a logger

* Fix linting errors arising from not using loggers

* Remove console calls from tests

Also removes some try/catch blocks that appeared to be concealing test
failures?

* fix non-camelCase component naming

Also alphabetically sorted the declarations for the core module - we
were declaring a few things multiple times.

* Simplify type guards in server configuration

Makes use of the utils package to avoid the need for 'as' statements

* Add typing for a Node SystemError

* Fix incorrect use of logging middleware

* Try to clean up some errors being hit in the SSR handler

limited success on that front

* fix directory handle leak

* Make non-production server builds debug-able

* add initial debug log for all requests, fix double-writing error responses in some cases

* Move "time elapsed" debug message so that non-error responses also log it

* Remove duplicated check, extraneous substring argument

* Fix missing `req` option to SSR engine handler

* Update a lot of fs access to be asynchronous

* Fix hard-coded VERSION file path

* Cache file compressions instead of recalculating on every request

Made it like 14x faster on my machine - I was a real idiot to not do
that in the first place.

* Fix lint errors from rebasing

* Make regexp non-polynomial

* Fix comment grammar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental a feature/component not directly supported by ATC low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution Traffic Portal v2 Related to the experimental Traffic Portal version 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants