-
Notifications
You must be signed in to change notification settings - Fork 296
Harden telemetry delivery and refine recipe metadata #2443
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
base: main
Are you sure you want to change the base?
Changes from all commits
fad52d8
fe680ac
8aa3d3d
166487c
a6ff131
15a5c31
eb524c7
862a9a1
86d4d02
99f0e17
788e91c
06f1252
42dbce5
d948cd9
1ff609a
8987669
ff60650
1cae73f
14130d4
d9247e2
befbb08
c90e653
51d08fd
38f118c
7427d75
2b63059
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| """High-level telemetry logger facade for easy usage.""" | ||
|
|
||
| import logging | ||
| import threading | ||
| import uuid | ||
| from typing import Any, Callable, Optional | ||
|
|
||
|
|
@@ -28,6 +29,7 @@ class TelemetryLogger: | |
|
|
||
| _instance: Optional["TelemetryLogger"] = None | ||
| _default_logger: Optional["TelemetryLogger"] = None | ||
| _singleton_lock = threading.RLock() | ||
| _logger: Optional[logging.Logger] = None | ||
| _logger_exporter: Optional[OneCollectorLogExporter] = None | ||
| _logger_provider: Optional[LoggerProvider] = None | ||
|
|
@@ -39,9 +41,10 @@ def __new__(cls, options: Optional[OneCollectorExporterOptions] = None): | |
| options: Exporter options (only used on first instantiation) | ||
|
|
||
| """ | ||
| if cls._instance is None: | ||
| cls._instance = super().__new__(cls) | ||
| cls._instance._initialize(options) | ||
| with cls._singleton_lock: | ||
| if cls._instance is None: | ||
| cls._instance = super().__new__(cls) | ||
| cls._instance._initialize(options) | ||
|
Comment on lines
+44
to
+47
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be worth considering an instance per thread rather than dealing with multi-threading/multi-process issues in all the telemetry APIs. Most of Olive is single-threaded so race-conditions would be limited in APIs but once we start collecting more granular telemetry, this might become an issue. |
||
|
|
||
| return cls._instance | ||
|
|
||
|
|
@@ -57,10 +60,13 @@ def _initialize(self, options: Optional[OneCollectorExporterOptions]) -> None: | |
| self._logger_exporter = OneCollectorLogExporter(options=options) | ||
|
|
||
| # Create logger provider | ||
| service_name = ( | ||
| options.service_name if options and options.service_name else __name__.split(".", maxsplit=1)[0] | ||
| ) | ||
| self._logger_provider = LoggerProvider( | ||
| resource=Resource.create( | ||
| { | ||
| "service.name": __name__.split(".", maxsplit=1)[0], | ||
| "service.name": service_name, | ||
| "service.version": VERSION, | ||
| "service.instance.id": str(uuid.uuid4()), # Unique instance ID; can double as session ID | ||
| } | ||
|
|
@@ -141,21 +147,27 @@ def shutdown(self) -> None: | |
| self._logger_provider.shutdown() | ||
|
|
||
| @classmethod | ||
| def get_default_logger(cls, connection_string: Optional[str] = None) -> "TelemetryLogger": | ||
| def get_default_logger( | ||
| cls, connection_string: Optional[str] = None, service_name: Optional[str] = None | ||
| ) -> "TelemetryLogger": | ||
| """Get or create the default telemetry logger. | ||
|
|
||
| Args: | ||
| connection_string: OneCollector connection string (only used on first call) | ||
| service_name: Logical application/service name for emitted telemetry (only used on first call) | ||
|
|
||
| Returns: | ||
| TelemetryLogger instance | ||
|
|
||
| """ | ||
| if cls._default_logger is None: | ||
| options = None | ||
| if connection_string: | ||
| options = OneCollectorExporterOptions(connection_string=connection_string) | ||
| cls._default_logger = cls(options=options) | ||
| with cls._singleton_lock: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are protecting two separate resources |
||
| if cls._default_logger is None: | ||
| options = None | ||
| if connection_string: | ||
| options = OneCollectorExporterOptions( | ||
| connection_string=connection_string, service_name=service_name | ||
| ) | ||
| cls._default_logger = cls(options=options) | ||
|
|
||
| return cls._default_logger | ||
|
|
||
|
|
@@ -167,17 +179,20 @@ def shutdown_default_logger(cls) -> None: | |
| cls._default_logger = None | ||
|
|
||
|
|
||
| def get_telemetry_logger(connection_string: Optional[str] = None) -> TelemetryLogger: | ||
| def get_telemetry_logger( | ||
| connection_string: Optional[str] = None, service_name: Optional[str] = None | ||
| ) -> TelemetryLogger: | ||
|
Comment on lines
+182
to
+184
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be a class method as well since all it is accessing is other class variables and methods. |
||
| """Get or create the default telemetry logger. | ||
|
|
||
| Args: | ||
| connection_string: OneCollector connection string (only used on first call) | ||
| service_name: Logical application/service name for emitted telemetry (only used on first call) | ||
|
|
||
| Returns: | ||
| TelemetryLogger instance | ||
|
|
||
| """ | ||
| return TelemetryLogger.get_default_logger(connection_string=connection_string) | ||
| return TelemetryLogger.get_default_logger(connection_string=connection_string, service_name=service_name) | ||
|
|
||
|
|
||
| def log_event(event_name: str, attributes: Optional[dict[str, Any]] = None) -> None: | ||
|
|
||
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.
You might want a deepcopy here