-
Notifications
You must be signed in to change notification settings - Fork 7
Fix initialization #97
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes initialization logic in the observability core module and prevents direct instantiation of the Agent365Exporter by marking it as an internal class. The key changes ensure that all span processors are properly added to the tracer provider and that the exporter is only accessible through the configuration API.
Key Changes:
- Fixed initialization logic to properly add both BatchSpanProcessor and SpanProcessor to the tracer provider
- Renamed
Agent365Exporterto_Agent365Exporterwith@finaldecorator to indicate it's internal - Added duplicate initialization prevention in the configure function
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
agent365_exporter.py |
Renamed class to _Agent365Exporter and added @final decorator to prevent inheritance and indicate internal use |
__init__.py |
Added export control to prevent external access to the exporter (only exports Agent365ExporterOptions) |
config.py |
Fixed initialization logic to add all processors correctly and prevent duplicate initialization; updated import to use _Agent365Exporter |
test_agent365_exporter.py |
Updated all test references from Agent365Exporter to _Agent365Exporter; added test to verify underscore prefix convention |
test_agent365.py |
Added comprehensive tests for initialization logic including duplicate prevention, new tracer provider creation, and existing provider usage |
Comments suppressed due to low confidence (1)
libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/agent365_exporter.py:47
- The docstring for this class should be updated to indicate that this is an internal/private class that should not be instantiated directly by developers. Consider adding a note like "Note: This is an internal class. Use the configure() function from the observability.core module instead of instantiating this class directly."
"""
Agent 365 span exporter for Agent 365:
* Partitions spans by (tenantId, agentId)
* Builds OTLP-like JSON: resourceSpans -> scopeSpans -> spans
* POSTs per group to https://{endpoint}/maven/agent365/agents/{agentId}/traces?api-version=1
* Adds Bearer token via token_resolver(agentId, tenantId)
"""
...gents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/__init__.py
Show resolved
Hide resolved
...gents-a365-observability-core/microsoft_agents_a365/observability/core/exporters/__init__.py
Show resolved
Hide resolved
.../microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/config.py
Outdated
Show resolved
Hide resolved
|
@nikhilNava I've opened a new pull request, #98, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@nikhilNava I've opened a new pull request, #99, to work on those changes. Once the pull request is ready, I'll request review from you. |
…#99) * Initial plan * Fix filename and add copyright header to exporters __init__.py Co-authored-by: nikhilNava <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: nikhilNava <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
.../microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/config.py
Outdated
Show resolved
Hide resolved
...5-observability-core/microsoft_agents_a365/observability/core/exporters/agent365_exporter.py
Show resolved
Hide resolved
...5-observability-core/microsoft_agents_a365/observability/core/exporters/agent365_exporter.py
Show resolved
Hide resolved
.../microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/config.py
Outdated
Show resolved
Hide resolved
|
@nikhilNava I've opened a new pull request, #100, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Remove trailing space from warning message Co-authored-by: nikhilNava <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: nikhilNava <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Task
Solution