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

GO-3753 Add write to file #1617

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

fat-fellow
Copy link
Contributor


  • I understand that contributing to this repository will require me to agree with the CLA

Description

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 README.md
  • 📓 tech-docs
  • 🙅 no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

@fat-fellow fat-fellow marked this pull request as draft September 26, 2024 14:29
Copy link

github-actions bot commented Sep 26, 2024

New Coverage 48.3% of statements
Patch Coverage 0.0% of changed statements (0/70)

Coverage provided by https://github.com/seriousben/go-patch-cover-action

@fat-fellow fat-fellow force-pushed the go-3753-write-logs-to-file branch 3 times, most recently from b61e969 to c43bf87 Compare September 30, 2024 15:56
@@ -0,0 +1,4 @@
package global
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we choose a different name? common? shared?
root/

Copy link
Member

Choose a reason for hiding this comment

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

environment

message SetParameters {
message Request {
option (no_auth) = true;
string platform = 1;
string version = 2;
string workdir = 3;
string logLevel = 4;
bool doNotSendLogs = 5;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we want to send logs by default

core/metrics.go Outdated
func (mw *Middleware) MetricsSetParameters(cctx context.Context, req *pb.RpcMetricsSetParametersRequest) *pb.RpcMetricsSetParametersResponse {
response := func(code pb.RpcMetricsSetParametersResponseErrorCode, err error) *pb.RpcMetricsSetParametersResponse {
m := &pb.RpcMetricsSetParametersResponse{Error: &pb.RpcMetricsSetParametersResponseError{Code: code}}
func (mw *Middleware) SetInitialParams(cctx context.Context, req *pb.RpcInitialSetParametersRequest) *pb.RpcInitialSetParametersResponse {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of setmetricsparams

core/metrics.go Outdated
errors.New("version is empty. Version must be in format: 1.0.0-optional-commit-hash-for-dev-builds"))
}
mw.applicationService.SetClientVersion(req.Platform, req.Version)

metrics.Service.SetPlatform(req.Platform)
metrics.Service.SetStartVersion(req.Version)
logging.Init(req.Workdir, req.LogLevel, !req.DoNotSendLogs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if workdir is empty we don't write the logs
if loglevel is empty we use defaultlog levels
if donotsend we don't send gelf

@fat-fellow fat-fellow marked this pull request as ready for review September 30, 2024 16:13
@fat-fellow fat-fellow force-pushed the go-3753-write-logs-to-file branch 2 times, most recently from c443bd9 to 4060d85 Compare October 1, 2024 11:49
@fat-fellow fat-fellow force-pushed the go-3753-write-logs-to-file branch 3 times, most recently from d72d765 to 46b837d Compare October 1, 2024 18:08
Add an opt-out for gelf
Add an opt-out for telemetry
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.

2 participants