Skip to content

Commit 398b345

Browse files
juliomenendezclaude
andcommitted
fix: Address PR review feedback from Copilot
- Make default endpoint protocol-aware: 4317 for gRPC, 4318 for HTTP - Add suppress_invoke_agent_input as explicit param on public configure() - Fix insecure default from False to True in brainstorm docs (BRAINSTORM.md, TLDR.md, ARCHITECTURE.md) to match implementation - Add tests for HTTP default endpoint and explicit endpoint override Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 900af51 commit 398b345

6 files changed

Lines changed: 31 additions & 10 deletions

File tree

docs/brainstorm/spectra-collector-integration/ARCHITECTURE.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ configure(exporter_options=SpectraExporterOptions(...))
5151
│ │ SpectraExporterOpts │ │ Agent365ExporterOptions │ │
5252
│ │ endpoint (4317) │ │ token_resolver │ │
5353
│ │ protocol (gRPC) │ │ cluster_category │ │
54-
│ │ insecure (false) │ │ use_s2s_endpoint │ │
54+
│ │ insecure (true) │ │ use_s2s_endpoint │ │
5555
│ │ batch settings │ │ batch settings │ │
5656
│ └────────┬────────────┘ └────────┬────────────────┘ │
5757
│ │ │ │
@@ -92,7 +92,7 @@ class SpectraExporterOptions:
9292
self,
9393
endpoint: str = "http://localhost:4317",
9494
protocol: str = "grpc",
95-
insecure: bool = False,
95+
insecure: bool = True,
9696
max_queue_size: int = 2048,
9797
scheduled_delay_ms: int = 5000,
9898
exporter_timeout_ms: int = 30000,
@@ -113,7 +113,7 @@ class SpectraExporterOptions:
113113
|-------|------|---------|-------------|
114114
| `endpoint` | `str` | `http://localhost:4317` | Spectra sidecar OTLP endpoint |
115115
| `protocol` | `str` | `grpc` | OTLP protocol: `"grpc"` or `"http"` |
116-
| `insecure` | `bool` | `False` | Whether to use insecure (no TLS) connection |
116+
| `insecure` | `bool` | `True` | Whether to use insecure (no TLS) connection |
117117
| `max_queue_size` | `int` | `2048` | Batch processor queue size |
118118
| `scheduled_delay_ms` | `int` | `5000` | Export interval (ms) |
119119
| `exporter_timeout_ms` | `int` | `30000` | Export timeout (ms) |
@@ -252,4 +252,4 @@ configure(
252252
| Sidecar not running → silent failure | Medium | OTLP exporter logs connection errors; document deployment prereqs |
253253
| Consumer passes both A365 env var + Spectra options | Low | Spectra options take precedence; env var ignored. Document. |
254254
| gRPC dependency not installed | Low | `opentelemetry-exporter-otlp` (core dep) includes both gRPC and HTTP |
255-
| `insecure=False` default may fail on plain HTTP localhost | Low | Document: set `insecure=True` if sidecar doesn't have TLS |
255+
| Consumer sets `insecure=False` for remote Spectra endpoint but forgets TLS setup | Low | Only relevant for non-sidecar deployments. Default `insecure=True` is correct for localhost. |

docs/brainstorm/spectra-collector-integration/BRAINSTORM.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,12 +110,12 @@ The `TelemetryManager.configure()` method (`config.py`) creates the exporter pip
110110
|------|----------|------------|
111111
| Sidecar not running → silent failure | Medium | OTLP exporter logs connection errors; document deployment prereqs |
112112
| Consumer passes both A365 env var + Spectra options | Low | Spectra options take precedence; env var ignored |
113-
| `insecure=False` default may fail on plain HTTP localhost | Low | Document: set `insecure=True` if sidecar doesn't have TLS |
113+
| Consumer sets `insecure=False` for remote Spectra endpoint but forgets TLS setup | Low | Only relevant for non-sidecar deployments. Default `insecure=True` is correct for localhost. |
114114

115115
### Open Questions (Resolved)
116116
1. **Union type vs separate param** → Union type on `exporter_options`
117117
2. **A365 env var behavior** → Ignored entirely when SpectraExporterOptions provided
118-
3. **Insecure config** → Exposed, defaults to `False`
118+
3. **Insecure config** → Exposed, defaults to `True` (localhost sidecar)
119119
4. **Protocol config** → Exposed (`"grpc"` or `"http"`), defaults to `"grpc"`
120120
5. **Package exports**`SpectraExporterOptions` added to `__init__.py`
121121
6. **Test strategy** → Mock `OTLPSpanExporter`

docs/brainstorm/spectra-collector-integration/TLDR.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ Weave/Copilot Cowork needs Spectra Collector as their telemetry destination. Spe
1313

1414
## How
1515

16-
- New `SpectraExporterOptions` class with sensible defaults for K8s sidecar (`http://localhost:4317`, gRPC, insecure=false)
16+
- New `SpectraExporterOptions` class with sensible defaults for K8s sidecar (`http://localhost:4317`, gRPC, insecure=true)
1717
- `configure(exporter_options=...)` accepts `Agent365ExporterOptions | SpectraExporterOptions` — type-based dispatch selects the exporter
1818
- When `SpectraExporterOptions` is provided, `ENABLE_A365_OBSERVABILITY_EXPORTER` env var is ignored
1919
- Under the hood, creates an `OTLPSpanExporter` pointed at the Spectra endpoint — no new dependencies

libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/config.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,7 @@ def configure(
272272
token_resolver: Callable[[str, str], str | None] | None = None,
273273
cluster_category: str = "prod",
274274
exporter_options: Agent365ExporterOptions | SpectraExporterOptions | None = None,
275+
suppress_invoke_agent_input: bool = False,
275276
**kwargs: Any,
276277
) -> bool:
277278
"""
@@ -287,6 +288,7 @@ def configure(
287288
:param exporter_options: Exporter configuration. Pass Agent365ExporterOptions for A365 API
288289
export, SpectraExporterOptions for Spectra Collector sidecar export, or None (default)
289290
to construct Agent365ExporterOptions from legacy parameters.
291+
:param suppress_invoke_agent_input: If True, suppress input messages for InvokeAgent spans.
290292
:return: True if configuration succeeded, False otherwise.
291293
"""
292294
return _telemetry_manager.configure(
@@ -296,6 +298,7 @@ def configure(
296298
token_resolver,
297299
cluster_category,
298300
exporter_options,
301+
suppress_invoke_agent_input,
299302
**kwargs,
300303
)
301304

libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/spectra_exporter_options.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@ class SpectraExporterOptions:
1717
options classes have no shared base class per design decision C4.
1818
"""
1919

20+
_DEFAULT_GRPC_ENDPOINT = "http://localhost:4317"
21+
_DEFAULT_HTTP_ENDPOINT = "http://localhost:4318"
22+
2023
def __init__(
2124
self,
22-
endpoint: str = "http://localhost:4317",
25+
endpoint: str | None = None,
2326
protocol: Literal["grpc", "http"] = "grpc",
2427
insecure: bool = True,
2528
max_queue_size: int = 2048,
@@ -29,7 +32,8 @@ def __init__(
2932
):
3033
"""
3134
Args:
32-
endpoint: Spectra sidecar OTLP endpoint. Default: http://localhost:4317.
35+
endpoint: Spectra sidecar OTLP endpoint. Defaults to
36+
http://localhost:4317 for gRPC or http://localhost:4318 for HTTP.
3337
protocol: OTLP protocol — "grpc" or "http". Default: grpc.
3438
insecure: Use insecure (no TLS) connection. Default: True (localhost sidecar).
3539
max_queue_size: Batch processor queue size. Default: 2048.
@@ -39,6 +43,10 @@ def __init__(
3943
"""
4044
if protocol not in ("grpc", "http"):
4145
raise ValueError(f"protocol must be 'grpc' or 'http', got '{protocol}'")
46+
if endpoint is None:
47+
endpoint = (
48+
self._DEFAULT_GRPC_ENDPOINT if protocol == "grpc" else self._DEFAULT_HTTP_ENDPOINT
49+
)
4250
self.endpoint = endpoint
4351
self.protocol = protocol
4452
self.insecure = insecure

tests/observability/core/test_spectra_exporter.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,16 @@ def test_spectra_exporter_options_defaults(self):
3939
self.assertEqual(opts.exporter_timeout_ms, 30000)
4040
self.assertEqual(opts.max_export_batch_size, 512)
4141

42+
def test_spectra_exporter_options_http_default_endpoint(self):
43+
"""HTTP protocol defaults to port 4318."""
44+
opts = SpectraExporterOptions(protocol="http")
45+
self.assertEqual(opts.endpoint, "http://localhost:4318")
46+
47+
def test_spectra_exporter_options_explicit_endpoint_overrides_default(self):
48+
"""Explicit endpoint overrides protocol-based default."""
49+
opts = SpectraExporterOptions(protocol="http", endpoint="http://custom:9999")
50+
self.assertEqual(opts.endpoint, "http://custom:9999")
51+
4252
def test_spectra_options_invalid_protocol_raises(self):
4353
"""ValueError for invalid protocol."""
4454
with self.assertRaises(ValueError) as ctx:
@@ -102,7 +112,7 @@ def test_configure_with_spectra_options_creates_http_exporter(self, mock_http):
102112
)
103113
self.assertTrue(result)
104114
mock_http.assert_called_once_with(
105-
endpoint="http://localhost:4317",
115+
endpoint="http://localhost:4318",
106116
)
107117

108118
@patch("microsoft_agents_a365.observability.core.config.GrpcOTLPSpanExporter")

0 commit comments

Comments
 (0)