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

fix: telemetry store #6923

Merged
merged 14 commits into from
Jan 30, 2025
Merged

fix: telemetry store #6923

merged 14 commits into from
Jan 30, 2025

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Jan 26, 2025

Fixes: https://github.com/SigNoz/platform-pod/issues/401

adds telemetry store package and use is as a part of signoz.

Major changes

  • The reason, telemtrystore provider is handing depricated ClickHouseUrl is because we need to parse it using clickhouse function.
  • Add telemetry store package which initalizes ch connection
  • the ch connection is the wrapper which adds default query settings as well.
  • removed old clickhouse connection configuration flags.
  • removed OptimizeReadInOrderRegex and OptimizeReadInOrderRegexCompiled from wrapper. as they are not used and were added during logs projections

Important

Introduces a new telemetry store package for ClickHouse connection management, replacing old configurations and handling deprecated flags for backward compatibility.

  • Behavior:
    • Introduces telemetrystore package for ClickHouse connection management.
    • Removes old ClickHouse connection flags and configurations.
    • Handles deprecated ClickHouseUrl and connection flags for backward compatibility.
  • Configuration:
    • Adds telemetrystore configuration to config.go.
    • Updates example.yaml with telemetrystore settings.
  • Server Initialization:
    • Updates NewServer in server.go to use telemetrystore for ClickHouse connections.
  • Testing:
    • Updates test cases in querier_test.go, v2/querier_test.go, and threshold_rule_test.go to use new telemetry store.
    • Adds mock telemetry store provider in telemetrystoretest package.

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

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the bug Something isn't working label Jan 26, 2025
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@nityanandagohain nityanandagohain marked this pull request as ready for review January 27, 2025 14:02
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 8f06f14 in 2 minutes and 14 seconds

More details
  • Looked at 1345 lines of code in 27 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pkg/telemetrystore/clickhousetelemetrystore/provider.go:28
  • Draft comment:
    Consider using SIGNOZ_TELEMETRYSTORE_CLICKHOUSE_DSN instead of SIGNOZ_TELEMETRYSTORE_CLICKHOUSE_ADDRESS for consistency with the rest of the configuration.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests changing a not-yet-implemented environment variable name. The code shows a TODO and is in transition from ClickHouseUrl to a new name. Without seeing the rest of the codebase's environment variable naming conventions, we can't be certain which name would be more consistent. The comment is speculative and premature since the new variable isn't even implemented yet.
    The comment could be valid if there's a strong environment variable naming convention in the codebase. The suggested name might be more technically accurate since it's a DSN not just an address.
    Without access to the broader codebase conventions, and given this is about a future implementation noted by a TODO, this comment is premature and speculative.
    Delete the comment as it's speculative and about a future implementation. The proper variable name should be decided when the TODO is addressed.
2. pkg/telemetrystore/telemetrystore.go:10
  • Draft comment:
    Ensure that the ClickHouseConn method has a clear implementation or intended use to avoid confusion.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. pkg/query-service/app/server.go:197
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets or styled components instead. This is also applicable in other parts of the codebase where inline styles are used.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_1aT3kAgodBZXOEm1


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

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 23e94cd in 44 seconds

More details
  • Looked at 42 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/telemetrystore/clickhousetelemetrystore/provider.go:25
  • Draft comment:
    Consider adding error handling for the clickhouse.Open function to handle connection failures gracefully.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
2. pkg/telemetrystore/clickhousetelemetrystore/provider.go:20
  • 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 the provider.go file.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_0vfzNmlKecIcx2hw


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

pkg/telemetrystore/telemetrystore.go Outdated Show resolved Hide resolved
pkg/telemetrystore/config.go Outdated Show resolved Hide resolved
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 0de2ec2 in 1 minute and 13 seconds

More details
  • Looked at 454 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pkg/telemetrystore/clickhousetelemetrystore/provider.go:83
  • Draft comment:
	telemetrystore.WrapAfterQuery(p.hooks, ctx, query, args, nil, row.Err())
  • Reason this comment was not posted:
    Marked as duplicate.
2. pkg/telemetrystore/clickhousetelemetrystore/provider.go:109
  • Draft comment:
	ctx, query, args := telemetrystore.WrapBeforeQuery(p.hooks, ctx, query, nil)
  • Reason this comment was not posted:
    Marked as duplicate.
3. pkg/telemetrystore/telemetrystorehook/clickhousesettings.go:82
  • Draft comment:
	logComment, err := json.Marshal(logCommentKVs)
	if err != nil {
		return ""
	}

- **Reason this comment was not posted:** 
Comment looked like it was already resolved.

</details>

<details>
<summary>4. <code>pkg/telemetrystore/clickhousetelemetrystore/provider.go:119</code></summary>

- **Draft comment:** 
The `Contributors` method should not be part of the `provider` struct as it is not related to ClickHouseReader interface. Access such information through the DAO in the telemetry instance instead.
- **Reason this comment was not posted:** 
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The Contributors() method is actually part of the clickhouse.Conn interface that this provider is implementing. It's not an arbitrary addition - it's implementing the full interface. The comment seems to misunderstand that this is a required part of the interface implementation. The rule about non-ClickHouse functions doesn't apply here since this IS a ClickHouse function.
I could be wrong about whether this method needs to be implemented - maybe there's a way to implement only part of the interface. I also don't have the full interface definition to confirm.
Since the provider struct is clearly implementing the full clickhouse.Conn interface (as shown by all the other method implementations), and Contributors() is part of that interface, removing it would break the interface implementation.
The comment should be deleted because it incorrectly suggests removing a method that is part of the required interface implementation.

</details>


Workflow ID: <workflowid>`wflow_NoG2JhCAqfT96Agw`</workflowid>

</details>


----
You can customize Ellipsis with :+1: / :-1: [feedback](https://docs.ellipsis.dev/review), review rules, user-specific overrides, `quiet` mode, and [more](https://docs.ellipsis.dev/config).

ee/query-service/main.go Outdated Show resolved Hide resolved
pkg/signoz/config.go Outdated Show resolved Hide resolved
pkg/signoz/config.go Outdated Show resolved Hide resolved
pkg/telemetrystore/telemetrystore.go Outdated Show resolved Hide resolved
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 2e713b4 in 42 seconds

More details
  • Looked at 149 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pkg/telemetrystore/telemetrystore.go:12
  • Draft comment:
    Consider renaming ClickHouseDB to GetClickHouseConn for clarity and consistency with its purpose of returning a ClickHouse connection. This applies to both telemetrystore.go and provider.go.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function name ClickHouseDB in telemetrystore.go and provider.go is inconsistent with the comment above it, which mentions 'SigNoz Wrapper for Clickhouse'. The function name should reflect its purpose more clearly.
2. pkg/signoz/config.go:44
  • Draft comment:
    The typo correction from DepricatedFlags to DeprecatedFlags is good. Ensure this change is consistently applied across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The PR includes a typo correction from DepricatedFlags to DeprecatedFlags. This is a good change for consistency and correctness. However, the change should be applied consistently across the codebase.
3. pkg/telemetrystore/clickhousetelemetrystore/provider.go:56
  • Draft comment:
    Ensure that the function name change from ClickHouse to ClickHouseDB is consistently applied across all files and usages to prevent runtime errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR changes the function name from ClickHouse to ClickHouseDB in multiple files. This change should be consistent across all usages to avoid any runtime errors.
4. pkg/telemetrystore/clickhousetelemetrystore/provider.go:56
  • Draft comment:
    Ensure that non-ClickHouse related functions are not added to the ClickHouseReader interface. Use the DAO in the telemetry instance for such information instead. This applies to pkg/telemetrystore/telemetrystore.go as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_Atta2AJXJUQKsXxe


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

@nityanandagohain nityanandagohain enabled auto-merge (squash) January 30, 2025 10:19
@nityanandagohain nityanandagohain merged commit d1e7cc1 into main Jan 30, 2025
17 checks passed
@nityanandagohain nityanandagohain deleted the issue_401 branch January 30, 2025 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants