You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "ocket8888 (via GitHub)" <gi...@apache.org> on 2023/07/25 18:59:18 UTC

[GitHub] [trafficcontrol] ocket8888 opened a new pull request, #7673: Tpv2 logging overhaul

ocket8888 opened a new pull request, #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673

   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).
   
   <hr/>
   
   ## 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
   - [x] This PR has tests
   - [x] This PR has documentation
   - [x] This PR doesn't need a CHANGELOG.md entry
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #7673: Tpv2 logging overhaul

Posted by "shamrickus (via GitHub)" <gi...@apache.org>.
shamrickus commented on code in PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673#discussion_r1283535045


##########
experimental/traffic-portal/src/app/shared/logging.service.ts:
##########
@@ -0,0 +1,74 @@
+/**
+ * @license Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Injectable } from "@angular/core";
+
+import { LogLevel, Logger } from "../utils";
+
+/**
+ * LoggingService is for logging things in a consistent way across the UI.
+ *
+ * It's basically just a thin wrapper around a {@link Logger} so that only one
+ * instance needs to exist and injection makes its setup consistent across all
+ * usages.
+ */
+@Injectable({
+	providedIn: "root"
+})
+export class LoggingService {
+
+	public logger: Logger;
+
+	constructor() {
+		this.logger = new Logger(console, LogLevel.DEBUG, "", false);

Review Comment:
   Shouldn't this `LogLevel` be dependent on the environment? Probably don't want debug statements in prod.



##########
experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.ts:
##########
@@ -78,12 +79,14 @@ export class ImportJsonTxtComponent {
 	 *
 	 * @param dialogRef Dialog manager
 	 * @param alertService Alert service manager
-	 * @param datePipe Default angular date pipe for formating date
+	 * @param datePipe Default angular date pipe for formatting date

Review Comment:
   This should probably read `formatting a date`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] shamrickus merged pull request #7673: Tpv2 logging overhaul

Posted by "shamrickus (via GitHub)" <gi...@apache.org>.
shamrickus merged PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7673: Tpv2 logging overhaul

Posted by "ocket8888 (via GitHub)" <gi...@apache.org>.
ocket8888 commented on code in PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673#discussion_r1288636853


##########
experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.ts:
##########
@@ -78,12 +79,14 @@ export class ImportJsonTxtComponent {
 	 *
 	 * @param dialogRef Dialog manager
 	 * @param alertService Alert service manager
-	 * @param datePipe Default angular date pipe for formating date
+	 * @param datePipe Default angular date pipe for formatting date

Review Comment:
   It should. I didn't write it though, but as long as I'm gonna bother fixing spelling I'll fix the grammar too.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7673: Tpv2 logging overhaul

Posted by "ocket8888 (via GitHub)" <gi...@apache.org>.
ocket8888 commented on code in PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673#discussion_r1283818560


##########
experimental/traffic-portal/src/app/shared/logging.service.ts:
##########
@@ -0,0 +1,74 @@
+/**
+ * @license Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Injectable } from "@angular/core";
+
+import { LogLevel, Logger } from "../utils";
+
+/**
+ * LoggingService is for logging things in a consistent way across the UI.
+ *
+ * It's basically just a thin wrapper around a {@link Logger} so that only one
+ * instance needs to exist and injection makes its setup consistent across all
+ * usages.
+ */
+@Injectable({
+	providedIn: "root"
+})
+export class LoggingService {
+
+	public logger: Logger;
+
+	constructor() {
+		this.logger = new Logger(console, LogLevel.DEBUG, "", false);

Review Comment:
   actually on client side I kind of do, because the only scenario in which we'd be looking at them is when someone reports something and we need to debug it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] github-code-scanning[bot] commented on a diff in pull request #7673: Tpv2 logging overhaul

Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673#discussion_r1273967515


##########
experimental/traffic-portal/src/app/utils/logging.ts:
##########
@@ -0,0 +1,227 @@
+/**
+ * @license Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * LogStreams are the underlying raw event writers used by {@link Logger}s. The
+ * simplest and most useful example of a LogStreams implementation is `console`.
+ */
+export interface LogStreams {
+	debug(...args: unknown[]): void;
+	info(...args: unknown[]): void;
+	error(...args: unknown[]): void;
+	warn(...args: unknown[]): void;
+}
+
+/**
+ * A LogLevel describes the verbosity of logging. Each level is cumulative,
+ * meaning that a logger set to some level will also log all of the levels above
+ * it.
+ */
+export const enum LogLevel {
+	/** Log only errors. */
+	ERROR,
+	/** Log warnings and errors. */
+	WARN,
+	/** Log informational messages, warnings, and errors. */
+	INFO,
+	/** Log debugging messages, informational messages, warnings, and errors. */
+	DEBUG,
+}
+
+/**
+ * Converts a log level to a human-readable string.
+ *
+ * @example
+ * console.log(logLevelToString(LogLevel.DEBUG));
+ * // Output:
+ * // DEBUG
+ *
+ * @param level The level to convert.
+ * @returns A string representation of `level`.
+ */
+export function logLevelToString(level: LogLevel): string {
+	switch(level) {
+		case LogLevel.DEBUG:
+			return "DEBUG";
+		case LogLevel.ERROR:
+			return "ERROR";
+		case LogLevel.INFO:
+			return "INFO";
+		case LogLevel.WARN:
+			return "WARN";
+	}
+}
+
+/**
+ * A Logger logs things. The output streams are customizable, mostly for testing
+ * but also in case we want to write directly to a file handle someday.
+ *
+ * The output format is a bit customizable, it allows for messages to be
+ * prefixed in a number of ways:
+ * - With the level at which the message was logged
+ * - With a timestamp for the time at which logging occurred (ISO format)
+ * - With some static string
+ *
+ * in that order. For example, if all of them are specified:
+ *
+ * @example
+ * (new Logger(console, LogLevel.DEBUG, "test", true, true)).info("quest");
+ * // Output (example date is UNIX epoch):
+ * // INFO 1970-01-01T00:00:00.000Z test: quest
+ */
+export class Logger {
+	private readonly prefix: string;
+
+	/**
+	 * Constructor.
+	 *
+	 * @param streams The output stream abstractions.
+	 * @param level The level at which the logger operates. Any level higher
+	 * than the one specified will not be logged.
+	 * @param prefix If given, prepends a prefix to each message.
+	 * @param useLevelPrefixes If true, log lines will be prefixed with the name
+	 * of the level at which they were logged (useful if all streams point to
+	 * the same file descriptor).
+	 * @param timestamps If true, each log line will be accompanied by a
+	 * timestamp prefix (note that the time is determined when logging occurs,
+	 * not necessarily when the logging method is called).
+	 */
+	constructor(
+		private readonly streams: LogStreams,
+		level: LogLevel,
+		prefix: string = "",
+		private readonly useLevelPrefixes: boolean = true,
+		private readonly timestamps: boolean = true,
+	) {
+		if (prefix) {
+			prefix = prefix.trim().replace(/:+$/, "").trimEnd();

Review Comment:
   ## Polynomial regular expression used on uncontrolled data
   
   This [regular expression](1) that depends on [a user-provided value](2) may run slow on strings with many repetitions of ':'.
   
   [Show more details](https://github.com/apache/trafficcontrol/security/code-scanning/294)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #7673: Tpv2 logging overhaul

Posted by "shamrickus (via GitHub)" <gi...@apache.org>.
shamrickus commented on code in PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673#discussion_r1284564581


##########
experimental/traffic-portal/src/app/shared/logging.service.ts:
##########
@@ -0,0 +1,74 @@
+/**
+ * @license Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Injectable } from "@angular/core";
+
+import { LogLevel, Logger } from "../utils";
+
+/**
+ * LoggingService is for logging things in a consistent way across the UI.
+ *
+ * It's basically just a thin wrapper around a {@link Logger} so that only one
+ * instance needs to exist and injection makes its setup consistent across all
+ * usages.
+ */
+@Injectable({
+	providedIn: "root"
+})
+export class LoggingService {
+
+	public logger: Logger;
+
+	constructor() {
+		this.logger = new Logger(console, LogLevel.DEBUG, "", false);

Review Comment:
   I'd argue that should at least be configurable (with debug not being the default), but as you've mentioned configuration is not part of this PR so this is fine for now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] codecov[bot] commented on pull request #7673: Tpv2 logging overhaul

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7673:
URL: https://github.com/apache/trafficcontrol/pull/7673#issuecomment-1651986720

   ## [Codecov](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#7673](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (51e65ff) into [master](https://app.codecov.io/gh/apache/trafficcontrol/commit/75ec56fa1bc4cf9c9e9ba54bde92197d4381649d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (75ec56f) will **increase** coverage by `0.43%`.
   > Report is 26 commits behind head on master.
   > The diff coverage is `77.97%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #7673      +/-   ##
   ============================================
   + Coverage     65.05%   65.49%   +0.43%     
     Complexity       98       98              
   ============================================
     Files           314      316       +2     
     Lines         12365    12499     +134     
     Branches        907      918      +11     
   ============================================
   + Hits           8044     8186     +142     
   + Misses         3968     3959       -9     
   - Partials        353      354       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | traffic_portal_v2 | `74.66% <77.67%> (+0.88%)` | :arrow_up: |
   | unit_tests | `74.66% <77.67%> (+0.88%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files Changed](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...al/src/app/shared/interceptor/error.interceptor.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvc2hhcmVkL2ludGVyY2VwdG9yL2Vycm9yLmludGVyY2VwdG9yLnRz) | `3.22% <0.00%> (-0.11%)` | :arrow_down: |
   | [...tal/traffic-portal/src/app/shared/shared.module.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvc2hhcmVkL3NoYXJlZC5tb2R1bGUudHM=) | `100.00% <ø> (ø)` | |
   | [experimental/traffic-portal/src/app/utils/index.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvdXRpbHMvaW5kZXgudHM=) | `100.00% <ø> (ø)` | |
   | [...src/app/core/cdns/cdn-table/cdn-table.component.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvY29yZS9jZG5zL2Nkbi10YWJsZS9jZG4tdGFibGUuY29tcG9uZW50LnRz) | `35.13% <20.00%> (+0.88%)` | :arrow_up: |
   | [...servers/server-details/server-details.component.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvY29yZS9zZXJ2ZXJzL3NlcnZlci1kZXRhaWxzL3NlcnZlci1kZXRhaWxzLmNvbXBvbmVudC50cw==) | `61.53% <27.27%> (+0.27%)` | :arrow_up: |
   | [...ob-dialog/new-invalidation-job-dialog.component.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvY29yZS9kZWxpdmVyeXNlcnZpY2UvaW52YWxpZGF0aW9uLWpvYnMvbmV3LWludmFsaWRhdGlvbi1qb2ItZGlhbG9nL25ldy1pbnZhbGlkYXRpb24tam9iLWRpYWxvZy5jb21wb25lbnQudHM=) | `94.91% <33.33%> (+0.08%)` | :arrow_up: |
   | [.../app/shared/theme-manager/theme-manager.service.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvc2hhcmVkL3RoZW1lLW1hbmFnZXIvdGhlbWUtbWFuYWdlci5zZXJ2aWNlLnRz) | `93.75% <33.33%> (ø)` | |
   | [...delivery-service/new-delivery-service.component.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvY29yZS9kZWxpdmVyeXNlcnZpY2UvbmV3LWRlbGl2ZXJ5LXNlcnZpY2UvbmV3LWRlbGl2ZXJ5LXNlcnZpY2UuY29tcG9uZW50LnRz) | `73.71% <40.00%> (ø)` | |
   | [...traffic-portal/src/app/api/testing/user.service.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvYXBpL3Rlc3RpbmcvdXNlci5zZXJ2aWNlLnRz) | `50.70% <50.00%> (+0.34%)` | :arrow_up: |
   | [.../core/deliveryservice/ds-card/ds-card.component.ts](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXhwZXJpbWVudGFsL3RyYWZmaWMtcG9ydGFsL3NyYy9hcHAvY29yZS9kZWxpdmVyeXNlcnZpY2UvZHMtY2FyZC9kcy1jYXJkLmNvbXBvbmVudC50cw==) | `86.20% <50.00%> (+4.23%)` | :arrow_up: |
   | ... and [48 more](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
   
   ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/trafficcontrol/pull/7673/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org