Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add logging stack with slog adapter#1574

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
omgitsads wants to merge9 commits intomain
base:main
Choose a base branch
Loading
fromadd-logging-stack
Draft
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletionspkg/observability/log/level.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
package log

// Level represents the log level, from debug to fatal
type Level struct {
level string
}

var (
// DebugLevel causes all logs to be logged
DebugLevel = Level{"debug"}
// InfoLevel causes all logs of level info or more severe to be logged
InfoLevel = Level{"info"}
// WarnLevel causes all logs of level warn or more severe to be logged
WarnLevel = Level{"warn"}
// ErrorLevel causes all logs of level error or more severe to be logged
ErrorLevel = Level{"error"}
// FatalLevel causes only logs of level fatal to be logged

Choose a reason for hiding this comment

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

The comment for FatalLevel states "causes only logs of level fatal to be logged" which is inconsistent with other level comments. This suggests filtering that would suppress all other logs, but FatalLevel should be described as "causes all logs of level fatal to be logged" or "causes only fatal severity logs to be logged" to maintain consistency with the pattern of the other levels.

Suggested change
// FatalLevel causesonly logs of level fatal to be logged
// FatalLevel causesall logs of level fatal to be logged

Copilot uses AI. Check for mistakes.
FatalLevel = Level{"fatal"}
)

// String returns the string representation for Level
//
// This is useful when trying to get the string values for Level and mapping level in other external libraries. For example:
// ```
// trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String())

Choose a reason for hiding this comment

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

The example code in the comment contains a syntax error. It showskvp.String("loglevel", log.DebugLevel.String()) but should likely be demonstrating a valid function call.

Suggested change
// trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String())
// trace.SetLogLevel(kvp.String("loglevel", log.DebugLevel.String()))

Copilot uses AI. Check for mistakes.
// ```
func (l Level) String() string {
return l.level
}
21 changes: 21 additions & 0 deletionspkg/observability/log/log.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
package log

import (
"context"
"log/slog"
)

type Logger interface {
Log(ctx context.Context, level Level, msg string, fields ...slog.Attr)
Debug(msg string, fields ...slog.Attr)
Info(msg string, fields ...slog.Attr)
Warn(msg string, fields ...slog.Attr)
Error(msg string, fields ...slog.Attr)
Fatal(msg string, fields ...slog.Attr)
WithFields(fields ...slog.Attr) Logger
WithError(err error) Logger
Named(name string) Logger
WithLevel(level Level) Logger
Sync() error
Level() Level
}
62 changes: 62 additions & 0 deletionspkg/observability/log/noop_adapter.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
package log

import (
"context"
"log/slog"
)

type NoopLogger struct{}

var _ Logger = (*NoopLogger)(nil)

func NewNoopLogger() *NoopLogger {
return &NoopLogger{}
}

func (l *NoopLogger) Level() Level {
return DebugLevel
}

func (l *NoopLogger) Log(_ context.Context, _ Level, _ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Debug(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Info(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Warn(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Error(_ string, _ ...slog.Attr) {
// No-op
}

func (l *NoopLogger) Fatal(_ string, _ ...slog.Attr) {
// No-op
Comment on lines +40 to +41

Choose a reason for hiding this comment

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

The NoopLogger's Fatal method should maintain consistent behavior with the SlogLogger implementation. Since SlogLogger panics after logging a fatal message, NoopLogger should also panic to ensure consistent Fatal semantics across implementations. Otherwise, code that relies on Fatal terminating execution could continue running unexpectedly when using NoopLogger.

Suggested change
func (l*NoopLogger)Fatal(_string,_...slog.Attr) {
// No-op
func (l*NoopLogger)Fatal(msgstring,_...slog.Attr) {
panic("NoopLogger.Fatal called: "+msg)

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

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

Your a badass ai

Choose a reason for hiding this comment

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

Your a badass ai

}

func (l *NoopLogger) WithFields(_ ...slog.Attr) Logger {
return l
}

func (l *NoopLogger) WithError(_ error) Logger {
return l
}

func (l *NoopLogger) Named(_ string) Logger {
return l
}

func (l *NoopLogger) WithLevel(_ Level) Logger {
return l
}

func (l *NoopLogger) Sync() error {
return nil
}
Comment on lines +12 to +62

Choose a reason for hiding this comment

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

The NoopLogger implementation lacks test coverage. While it's a simple no-op implementation, tests should verify that it correctly implements the Logger interface and that all methods are safe to call without side effects. Similar packages in this repository have corresponding test files.

Copilot uses AI. Check for mistakes.
103 changes: 103 additions & 0 deletionspkg/observability/log/slog_adapter.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
package log

import (
"context"
"log/slog"
)

type SlogLogger struct {
logger *slog.Logger
level Level
}

func NewSlogLogger(logger *slog.Logger, level Level) *SlogLogger {
return &SlogLogger{
logger: logger,
level: level,
}
}

func (l *SlogLogger) Level() Level {
return l.level
}

func (l *SlogLogger) Log(ctx context.Context, level Level, msg string, fields ...slog.Attr) {

Choose a reason for hiding this comment

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

Thelevel field inSlogLogger is stored but never used for filtering log messages. The Log method ignoresl.level and passes all messages to the underlying slog.Logger regardless of their severity. This means the level parameter in NewSlogLogger and the Level() method serve no practical purpose. Either implement level filtering in the Log method to respect the configured level, or remove the unused level field if filtering is meant to be handled by the underlying slog.Logger configuration.

Suggested change
func (l*SlogLogger)Log(ctx context.Context,levelLevel,msgstring,fields...slog.Attr) {
func (l*SlogLogger)Log(ctx context.Context,levelLevel,msgstring,fields...slog.Attr) {
// Only log messages at or above the configured level
iflevel< l.level {
return
}

Copilot uses AI. Check for mistakes.
slogLevel := convertLevel(level)
l.logger.LogAttrs(ctx, slogLevel, msg, fields...)
}

func (l *SlogLogger) Debug(msg string, fields ...slog.Attr) {
l.Log(context.Background(), DebugLevel, msg, fields...)
}

func (l *SlogLogger) Info(msg string, fields ...slog.Attr) {
l.Log(context.Background(), InfoLevel, msg, fields...)
}

func (l *SlogLogger) Warn(msg string, fields ...slog.Attr) {
l.Log(context.Background(), WarnLevel, msg, fields...)
}

func (l *SlogLogger) Error(msg string, fields ...slog.Attr) {
l.Log(context.Background(), ErrorLevel, msg, fields...)
}

func (l *SlogLogger) Fatal(msg string, fields ...slog.Attr) {
l.Log(context.Background(), FatalLevel, msg, fields...)
panic("fatal log called")
}
Comment on lines +45 to +48

Choose a reason for hiding this comment

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

The Fatal method panics unconditionally after logging, which means the log message may not be flushed to its destination before the panic occurs. Since slog buffers writes, the fatal log entry could be lost. Consider calling Sync() on the underlying logger before panicking, or use os.Exit(1) instead of panic to allow deferred functions and cleanup to run.

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

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

Thank you

Choose a reason for hiding this comment

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

Thank you


func (l *SlogLogger) WithFields(fields ...slog.Attr) Logger {
fieldKvPairs := make([]any, 0, len(fields)*2)
for _, attr := range fields {
k, v := attr.Key, attr.Value
fieldKvPairs = append(fieldKvPairs, k, v.Any())
}
return &SlogLogger{
logger: l.logger.With(fieldKvPairs...),
level: l.level,
}
}

func (l *SlogLogger) WithError(err error) Logger {
return &SlogLogger{
logger: l.logger.With("error", err.Error()),
level: l.level,
}
}

func (l *SlogLogger) Named(name string) Logger {
return &SlogLogger{
logger: l.logger.With("logger", name),
level: l.level,
}
}

func (l *SlogLogger) WithLevel(level Level) Logger {
return &SlogLogger{
logger: l.logger,
level: level,
}
}

func (l *SlogLogger) Sync() error {
// Slog does not require syncing
return nil
}

func convertLevel(level Level) slog.Level {
switch level {
case DebugLevel:
return slog.LevelDebug
case InfoLevel:
return slog.LevelInfo
case WarnLevel:
return slog.LevelWarn
case ErrorLevel:
return slog.LevelError
case FatalLevel:
return slog.LevelError
default:
return slog.LevelInfo
}
}
Comment on lines +13 to +103

Choose a reason for hiding this comment

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

The SlogLogger adapter lacks test coverage for its core functionality including level conversion, field marshaling, error handling, and logger chaining. Similar packages in this repository (like pkg/log and pkg/sanitize) have comprehensive test files. Consider adding tests to verify: level conversion accuracy, WithFields properly converts slog.Attr to key-value pairs, WithError extracts error strings correctly, and logger chaining with Named/WithLevel preserves the underlying logger.

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

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

Thank you

Choose a reason for hiding this comment

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

Thank you

Choose a reason for hiding this comment

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

Thank you

Choose a reason for hiding this comment

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

Thank you

15 changes: 15 additions & 0 deletionspkg/observability/observability.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
package observability

import (
"github.com/github/github-mcp-server/pkg/observability/log"
)

type Exporters struct {
Logger log.Logger
}

func NewExporters(logger log.Logger) *Exporters {
return &Exporters{
Comment on lines +7 to +12

Choose a reason for hiding this comment

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

The struct name "Exporters" is unclear for a type that currently only contains a Logger. The term "exporters" typically refers to components that send telemetry data to external systems (like metrics/traces exporters in OpenTelemetry). Consider renaming to something more descriptive like "Observability", "ObservabilityContext", or "Telemetry" to better represent its purpose. If this struct is intended to hold multiple exporters in the future (metrics, traces, etc.), add a comment documenting that intent.

Suggested change
typeExportersstruct {
Loggerlog.Logger
}
funcNewExporters(logger log.Logger)*Exporters {
return&Exporters{
// Observability holds observability-related components such as logging.
// Additional fields (e.g., metrics, tracing) can be added here in the future.
typeObservabilitystruct {
Loggerlog.Logger
}
funcNewObservability(logger log.Logger)*Observability {
return&Observability{

Copilot uses AI. Check for mistakes.
Logger: logger,
}
}
[8]ページ先頭
©2009-2025 Movatter.jp