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

ENG-36756: add logging config #18

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Conversation

kaushal02
Copy link
Contributor

@tim-mwangi
Copy link
Contributor

Hey @kaushal02 generated code at gen/go/v1/config.pb.go should be checked into the PR.

// logging level. Allowed values are trace, debug, info, warn, error, critical.
google.protobuf.StringValue level = 2;
// if logging mode is file, provide this additional configuration
LogFileConfig log_file = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering if we should rename this to log_rotation so that its consistent with TPA and eBPF? https://github.com/Traceableai/traceable-agent/blob/056b2578d7b24a0c643e99b29e2192de8ab9a2cb/proto/config/common/v1/config.proto#L135

Choose a reason for hiding this comment

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

I feel that rotation is separate to file logging. that way file name is a separate config and log_rotation is separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

like

logging:
- level
- mode
- filename

and

log_rotation:
- max_file_size
- max_file_count

?

Choose a reason for hiding this comment

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

does it support oneof?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it does

Choose a reason for hiding this comment

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

Current config looks good to me.

@kaushal02 kaushal02 merged commit ca69f0d into main Dec 19, 2023
1 check passed
// logging mode. Allowed values are none, stdout, file.
google.protobuf.StringValue mode = 1;
// logging level. Allowed values are trace, debug, info, warn, error, critical.
google.protobuf.StringValue level = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaushal02 can we use enum instead of strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't realise that was an option. Updating it now, thanks :)

@ryanericson ryanericson deleted the ENG-36756-add-logging-proto branch December 21, 2023 03:49
@kaushal02 kaushal02 mentioned this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants