Skip to content

Fix nil returns from BaseDeps.Logger and BaseDeps.Metrics when Obsv is nil#2264

Merged
mattdholloway merged 4 commits intoadd-logging-stack-v2from
copilot/sub-pr-2008
Mar 26, 2026
Merged

Fix nil returns from BaseDeps.Logger and BaseDeps.Metrics when Obsv is nil#2264
mattdholloway merged 4 commits intoadd-logging-stack-v2from
copilot/sub-pr-2008

Conversation

Copy link
Contributor

Copilot AI commented Mar 26, 2026

Summary

BaseDeps.Logger() and BaseDeps.Metrics() returned nil when Obsv is nil, creating an inconsistent contract that invited nil-pointer panics at any call site. This PR ensures both methods always return safe, functional defaults.

Why

ToolDependencies.Logger(ctx) and ToolDependencies.Metrics(ctx) are expected to be callable without nil checks at call sites — returning nil violated that contract. Many tests construct BaseDeps as partial struct literals without setting Obsv, making this a practical risk.

What changed

  • BaseDeps.Logger(): returns slog.New(slog.DiscardHandler) instead of nil when Obsv == nil
  • BaseDeps.Metrics(): returns metrics.NewNoopMetrics() instead of nil when Obsv == nil
  • pkg/http/server.go: passes metrics.NewNoopMetrics() explicitly to observability.NewExporters() instead of nil, ensuring the resulting Exporters has a valid metrics implementation throughout

MCP impact

  • No tool or API changes

Prompts tested (tool changes only)

Security / limits

  • No security or limits impact

Tool renaming

  • I am not renaming tools as part of this PR

Lint & tests

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed

📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

… Obsv is nil

Agent-Logs-Url: https://github.com/github/github-mcp-server/sessions/53221b0b-abb4-4138-a147-3ce9e13b379a

Co-authored-by: mattdholloway <918573+mattdholloway@users.noreply.github.com>
@mattdholloway mattdholloway marked this pull request as ready for review March 26, 2026 15:27
@mattdholloway mattdholloway requested a review from a team as a code owner March 26, 2026 15:27
Copilot AI review requested due to automatic review settings March 26, 2026 15:27
@mattdholloway mattdholloway merged commit 1ae4a03 into add-logging-stack-v2 Mar 26, 2026
8 checks passed
@mattdholloway mattdholloway deleted the copilot/sub-pr-2008 branch March 26, 2026 15:28
Copy link
Contributor Author

Copilot AI commented Mar 26, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • golangci-lint.run
    • Triggering command: /usr/bin/curl curl -sSfL REDACTED (dns block)
  • raw.ghe.example.com
    • Triggering command: /tmp/go-build2919302833/b342/oauth.test /tmp/go-build2919302833/b342/oauth.test -test.testlogfile=/tmp/go-build2919302833/b342/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� x64/src/runtime/cgo x64/src/crypto/internal/fips140/github.com/github/github-mcp-server/pkg/raw ux_amd64/vet (dns block)
  • raw.mycompanygithub.com
    • Triggering command: /tmp/go-build2919302833/b380/utils.test /tmp/go-build2919302833/b380/utils.test -test.testlogfile=/tmp/go-build2919302833/b380/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� _.a x64/src/crypto/x509/pkix/pkix.go ux_amd64/vet (dns block)
  • raw.myghe.com
    • Triggering command: /tmp/go-build2919302833/b380/utils.test /tmp/go-build2919302833/b380/utils.test -test.testlogfile=/tmp/go-build2919302833/b380/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� _.a x64/src/crypto/x509/pkix/pkix.go ux_amd64/vet (dns block)
  • raw.notgithub.com
    • Triggering command: /tmp/go-build2919302833/b380/utils.test /tmp/go-build2919302833/b380/utils.test -test.testlogfile=/tmp/go-build2919302833/b380/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� _.a x64/src/crypto/x509/pkix/pkix.go ux_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] [WIP] Address feedback on OSS logging adapter for http implementation Fix nil returns from BaseDeps.Logger and BaseDeps.Metrics when Obsv is nil Mar 26, 2026
Copilot AI requested a review from mattdholloway March 26, 2026 15:28
Copilot stopped work on behalf of mattdholloway due to an error March 26, 2026 15:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Aligns the HTTP server’s observability wiring and ToolDependencies defaults with the newer observability abstraction introduced in #2008, ensuring logger/metrics access is nil-safe across dependency injection paths.

Changes:

  • Initialize HTTP server observability exporters with metrics.NewNoopMetrics() instead of nil.
  • Make github.BaseDeps return a discard logger and no-op metrics when Obsv is unset (nil) to avoid nil returns from ToolDependencies.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/http/server.go Uses a no-op metrics implementation when constructing observability.Exporters for the HTTP server.
pkg/github/dependencies.go Ensures BaseDeps.Logger() / BaseDeps.Metrics() are nil-safe when Obsv is nil.

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