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

remove old logger from local server, use slog and span http, add span_id and trace_sampled attrs to log #1457

Merged

Conversation

James-Pickett
Copy link
Contributor

@James-Pickett James-Pickett commented Nov 14, 2023

  • uses new slogger throughout local server
  • add helper func to traces to add trace info to request ctx
  • adds trace_id to log shipping
  • adds trace_sampled value to log shipping

relates to #1449

@James-Pickett James-Pickett marked this pull request as ready for review November 14, 2023 17:45
pkg/traces/traces.go Show resolved Hide resolved
ee/localserver/logging-handler.go Outdated Show resolved Hide resolved
ee/localserver/server.go Outdated Show resolved Hide resolved
@@ -155,17 +142,29 @@ func (ls *localServer) LoadDefaultKeyIfNotSet() error {

serverRsaCertPem := k2RsaServerCert
serverEccCertPem := k2EccServerCert

ctx := context.TODO()
slogLevel := slog.LevelDebug
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anything change this?

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, it just felt correct to have it in one spot incase we wanted to change it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you don't plan on implementing that, it's more confusing than helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove the var next time I touch this area.

RebeccaMahany
RebeccaMahany previously approved these changes Nov 14, 2023
@James-Pickett James-Pickett changed the title remove old logger from local server, use slog and span http remove old logger from local server, use slog and span http, add span_id and trace_sampled attrs to log Nov 14, 2023
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Woo

@@ -155,17 +142,29 @@ func (ls *localServer) LoadDefaultKeyIfNotSet() error {

serverRsaCertPem := k2RsaServerCert
serverEccCertPem := k2EccServerCert

ctx := context.TODO()
slogLevel := slog.LevelDebug
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you don't plan on implementing that, it's more confusing than helpful.

@James-Pickett James-Pickett added this pull request to the merge queue Nov 15, 2023
Merged via the queue into kolide:main with commit 5b9a7f4 Nov 15, 2023
26 checks passed
@James-Pickett James-Pickett deleted the james/more-local-server-slogging branch November 15, 2023 15:48
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.

3 participants