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.

Sign up for GitHub

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

Jump to bottom

Use syslog format for logging, add service request scopes to logs #314

Merged
merged 10 commits into from
Nov 27, 2023
Merged

Use syslog format for logging, add service request scopes to logs #314

merged 10 commits into from
Nov 27, 2023

Conversation

mregen
Copy link
Contributor

@mregen mregen commented Nov 23, 2023 •
edited
Loading

Purpose

  • switch logging to ILogger syslog style format. new levels: critical, warn, info, debug, trace Default: info"
  • add scopes for ChannelId:
<4>2023-11-24T10:38:32.596Z [opcua@311 ChannelId:"13"] - Validation errors suppressed:  [CN=UACloudPublisher, C=US, S=Arizona, O=OPCFoundation] [DA9411A1308BF624E505E3A33A876EED1CC9A8BF]
<6>2023-11-24T10:38:32.609Z [opcua@311 ChannelId:"13"] - Session CREATED, Id=ns=8;i=1700941267, Name=UACloudPublisher, ChannelId=f53b64f8-afc5-4ca0-bb96-52009693223c-13, User=Anonymous
<6>2023-11-24T10:38:32.624Z [opcua@311 ChannelId:"13"] - Server - SESSION CREATED. SessionId=ns=8;i=1700941267
<6>2023-11-24T10:38:32.665Z [opcua@311 ChannelId:"13"] - Session VALIDATED, Id=ns=8;i=1700941267, Name=UACloudPublisher, ChannelId=f53b64f8-afc5-4ca0-bb96-52009693223c-13, User=Anonymous
<
  • add .editorconfig, fix new issues / warnings
  • Logger is now an Microsoft.Extensions.Logging.ILogger -> all log messages changed
  • OpcStackTraceMask is ignored by ILogger, removed

Does this introduce a breaking change?

[ ] Yes
[ ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

@mregen mregen marked this pull request as draft November 23, 2023 10:27
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 23, 2023
View reviewed changes
src/OpcApplicationConfigurationSecurity.cs Dismissed Show dismissed Hide dismissed
@mregen mregen marked this pull request as ready for review November 24, 2023 11:08
@luiscantero
Copy link
Collaborator

luiscantero commented Nov 27, 2023 •
edited
Loading

[*.cs]

Why move this file from root?
Now it doesn't apply to tests etc. #Closed

Refers to: .editorconfig:1 in 713f9bd. [](commit_id = 713f9bd, deletion_comment = True)

luiscantero
luiscantero reviewed Nov 27, 2023
View reviewed changes
src/CliOptions.cs Outdated
@@ -31,11 +32,11 @@ public static Mono.Options.OptionSet InitCommandLineOptions()
}
}
},
{ "ll|loglevel=", "the loglevel to use (allowed: fatal, error, warn, info, debug, verbose).\nDefault: info", (string s) => {
var logLevels = new List<string> {"fatal", "error", "warn", "info", "debug", "verbose"};
{ "ll|loglevel=", "the loglevel to use (allowed: critical, warn, info, debug, trace Default: info", (string s) => {
Copy link
Collaborator

@luiscantero luiscantero Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Pls add \n just before default to be consistent with the rest below #Closed

luiscantero
luiscantero reviewed Nov 27, 2023
View reviewed changes
src/PlcNodeManager.cs
@@ -182,7 +183,7 @@ private BaseDataVariableState CreateBaseVariable(BaseDataVariableState baseDataV
}
else
{
Logger.Debug("NodeId type is {nodeIdType}", path.GetType().ToString());
Logger.LogDebug("NodeId type is {nodeIdType}", (string)path.GetType().ToString());
Copy link
Collaborator

@luiscantero luiscantero Nov 27, 2023 •
edited by mregen
Loading

Choose a reason for hiding this comment

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

(string)

Is this needed? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise logdebug is unhappy

luiscantero
luiscantero reviewed Nov 27, 2023
View reviewed changes
src/PlcServer.cs
asyncResult.AsyncState is object[] asyncStateArray &&
asyncStateArray[0] is TcpServerChannel channel)
{
using var scope = Logger.BeginScope("ChannelId:\"{ChannelId}\"", channel.Id);
Copy link
Collaborator

@luiscantero luiscantero Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"

Nit: add space after : #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the format goes directly in syslog and there is no space after :

luiscantero
luiscantero reviewed Nov 27, 2023
View reviewed changes
src/Program.cs
@@ -27,10 +29,15 @@ public static class Program
/// </summary>
public const string ProgramName = "OpcPlc";

/// <summary>
///
/// </summary>
Copy link
Collaborator

@luiscantero luiscantero Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Summary is empty #Closed

luiscantero
luiscantero reviewed Nov 27, 2023
View reviewed changes
src/OpcApplicationConfiguration.cs Outdated

var microsoftLogger = new SerilogLoggerFactory(Logger)
.CreateLogger("OPC");
var microsoftLogger = Program.LoggerFactory.CreateLogger("Opc");
Copy link
Collaborator

@luiscantero luiscantero Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Opc

Could we call this OpcUa? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@luiscantero
Copy link
Collaborator

luiscantero commented Nov 27, 2023 •
edited by mregen
Loading

@mregen also, pls increase version in version.json #Resolved

@mregen
Copy link
Contributor Author

mregen commented Nov 27, 2023

[*.cs]

good catch, this was unintentional...

In reply to: 1827222711

Refers to: .editorconfig:1 in 713f9bd. [](commit_id = 713f9bd, deletion_comment = True)

@mregen
Copy link
Contributor Author

mregen commented Nov 27, 2023

2.9.14

In reply to: 1827528585

luiscantero
luiscantero approved these changes Nov 27, 2023
View reviewed changes
Copy link
Collaborator

@luiscantero luiscantero left a comment

Choose a reason for hiding this comment

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

:shipit:

@luiscantero luiscantero merged commit b2867f3 into main Nov 27, 2023
14 checks passed
@luiscantero luiscantero deleted the syslogs branch November 27, 2023 12:48
@Scarjit Scarjit mentioned this pull request Nov 28, 2023
Closed
@luiscantero luiscantero mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luiscantero luiscantero luiscantero approved these changes

Assignees

@mregen mregen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mregen @luiscantero