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

feat(instrumentation): use new config factory #6834

Merged
merged 8 commits into from
Jan 17, 2025
Merged

Conversation

grandwizard28
Copy link
Collaborator

@grandwizard28 grandwizard28 commented Jan 16, 2025

Summary

Use new config factory and remove redundant configuration possibilities from the upstream

Related Issues / PR's

Part of #6782


Important

Introduce new configuration factory pattern and refactor instrumentation to use it, removing redundant configurations.

  • Behavior:
    • Use new ConfigFactory for creating configurations in pkg/instrumentation/config.go.
    • Remove instrumentation from default configs in pkg/config/config.go.
    • Delete unmarshaler_test.go as it's no longer needed.
  • Factory:
    • Add ConfigFactory, ProviderFactory, and Service interfaces in pkg/factory.
    • Implement NewConfigFactory, NewProviderFactory, and NewNamedMap for managing configurations and providers.
    • Add tests for new factory functionalities in pkg/factory.
  • Instrumentation:
    • Refactor SDK struct to implement Instrumentation and Service interfaces.
    • Add noopInstrumentation for testing purposes in pkg/instrumentation/instrumentationtest.
    • Update logger creation to use new factory pattern.

This description was created by Ellipsis for 6680622. It will automatically update as commits are pushed.

@github-actions github-actions bot added the enhancement New feature or request label Jan 16, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 513629e in 1 minute and 25 seconds

More details
  • Looked at 1110 lines of code in 19 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pkg/instrumentation/config.go:55
  • Draft comment:
    Consider making the config factory name dynamic or configurable if future requirements demand it. Currently, it's hardcoded as "instrumentation".
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The NewConfigFactory function in pkg/instrumentation/config.go is creating a new config factory with a hardcoded name "instrumentation". This could be a potential issue if the name needs to be dynamic or configurable in the future. However, since the PR description does not mention any requirement for dynamic naming, this is not a critical issue but something to keep in mind for future scalability.
2. pkg/instrumentation/instrumentation.go:31
  • Draft comment:
    Ensure consistent usage of mergeAttributes to avoid attribute conflicts or overwrites.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The mergeAttributes function in pkg/instrumentation/instrumentation.go is a utility function that merges two sets of attributes. It seems to be correctly implemented, but it's important to ensure that the function is used consistently across the codebase to avoid any attribute conflicts or overwrites.
3. pkg/instrumentation/instrumentation.go:48
  • Draft comment:
    Ensure that the logger configuration in newLogger aligns with the overall logging strategy of the application.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The newLogger function in pkg/instrumentation/instrumentation.go creates a new Zap logger with a JSON encoder for stdout and an OpenTelemetry bridge. The function seems well-implemented, but it's important to ensure that the logger configuration aligns with the overall logging strategy of the application.
4. pkg/factory/config.go:1
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable to other files in this PR as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_p1tGMX4lUzz1J4tK


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 6680622 in 17 seconds

More details
  • Looked at 37 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_y8xBvB25S4kotZGQ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@grandwizard28 grandwizard28 merged commit 939ab52 into main Jan 17, 2025
16 checks passed
@grandwizard28 grandwizard28 deleted the instrumentation branch January 17, 2025 09:24
amlannandy pushed a commit that referenced this pull request Jan 21, 2025
### Summary

Use new config factory and remove redundant configuration possibilities from the upstream
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants