Skip to content
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

Initialize the new log service #476

Merged
merged 4 commits into from
Mar 6, 2024
Merged

Conversation

leandro-godon
Copy link
Contributor

Goal

Add LogService and LogOrchestrator to the Log module so they can be used within the SDK.

@leandro-godon leandro-godon requested a review from a team as a code owner February 29, 2024 12:15
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

The addition to the module looks good but I have a question about whether it's necessary to create 2 additional background threads for this functionality

Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM pending one comment

@@ -63,4 +69,23 @@ internal class CustomerLogModuleImpl(
workerThreadModule.backgroundWorker(WorkerName.REMOTE_LOGGING)
)
}

override val logService: LogService by singleton {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might need to initialize this in the OpenTelemetryModule instead as it will be used for initializing other data capture sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the LogService needs to be used by other data sources, since it deals with Embrace logs only. Other sources can use the LogWriter which is initialized in the OpenTelemetryModule. On the other hand, LogOrchestrator might need to be moved, although its dependency on DeliveryService can complicate things.

Base automatically changed from leandro/log_service_event_data to master March 4, 2024 11:32
@leandro-godon leandro-godon merged commit 22e6b8a into master Mar 6, 2024
3 checks passed
@leandro-godon leandro-godon deleted the leandro/log_service_init branch March 6, 2024 14:48
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.

2 participants