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: support for new enrichment logic in traces #6438

Merged
merged 5 commits into from
Nov 16, 2024

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Nov 14, 2024

Support for new enrichment logic.

  • Support for multiple type/datatype as in logs.
  • Support for the new and old formats. Old format and static ones are served though the map in the constants file.
  • serviceName is replaced with service.Name in filters.

Part of #5713


Missing scope:-

  • There will be a supporting PR after this which will take care of getting all the keys from the DB as truth with the context that if it is materialized or not, attribute/resource and the corresponding datatype.
  • V2 of function in clickhouse reader that uses the new table.
  • Integration of the flag to use the new enrichment logic and the query builder.
    • every place the builder is used.

In future we can try to merge the enrich logic of logs and traces but for now I am keeping it separete.


Important

Adds new enrichment logic for traces, supporting multiple data types and formats, with updated tests and constants.

  • Behavior:
    • Adds new enrichment logic for traces in enrich.go, supporting multiple data types and formats.
    • Replaces serviceName with service.name in trace filters.
  • Functions:
    • Introduces EnrichTracesQuery() and enrichKeyWithMetadata() in enrich.go for trace enrichment.
    • Modifies GenerateLogEnrichmentKeys() to GenerateEnrichmentKeys() in logs.go.
  • Constants:
    • Updates StaticFieldsTraces in constants.go with new fields like response_status_code and external_http_url.
  • Tests:
    • Adds enrich_test.go for testing trace enrichment logic.
    • Updates query_builder_test.go with new test cases for trace queries.

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

@github-actions github-actions bot added docs required enhancement New feature or request labels Nov 14, 2024
@SigNoz SigNoz deleted a comment from github-actions bot Nov 14, 2024
@nityanandagohain nityanandagohain marked this pull request as ready for review November 14, 2024 10:06
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.

❌ Changes requested. Reviewed everything up to f56c532 in 1 minute and 29 seconds

More details
  • Looked at 477 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pkg/query-service/app/traces/v4/enrich.go:14
  • Draft comment:
    The isEnriched function has similar logic to the one in pkg/query-service/app/logs/v3/enrich_query.go. Consider refactoring to a common utility function to avoid code duplication.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function isEnriched in pkg/query-service/app/traces/v4/enrich.go and pkg/query-service/app/logs/v3/enrich_query.go have similar logic but are implemented separately. This could lead to code duplication and maintenance challenges. Consider refactoring to a common utility function.
2. pkg/query-service/app/traces/v4/enrich.go:28
  • Draft comment:
    The enrichKeyWithMetadata function has similar logic to enrichFieldWithMetadata in pkg/query-service/app/logs/v3/enrich_query.go. Consider refactoring to a common utility function to avoid code duplication.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The enrichKeyWithMetadata function in pkg/query-service/app/traces/v4/enrich.go and enrichFieldWithMetadata in pkg/query-service/app/logs/v3/enrich_query.go have similar logic. Consider refactoring to a common utility function to avoid code duplication.
3. pkg/query-service/utils/logs.go:43
  • Draft comment:
    The comment still references Test_GenerateLogEnrichmentKeys. Update it to Test_GenerateEnrichmentKeys for consistency.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
4. pkg/query-service/app/traces/v4/enrich.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.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is not relevant to the changes made in the diff. The file is a Go file, and there is no indication of a component/index.tsx structure being used. The comment seems misplaced.
    I might be missing some context about the project structure, but based on the provided diff, the comment does not seem applicable.
    The comment is about a file structure approach that is not evident in the provided Go file. It seems to be misplaced or irrelevant to the current changes.
    The comment is not relevant to the changes made in the diff and should be deleted.
5. pkg/query-service/app/traces/v4/enrich.go:28
  • Draft comment:
    This function appears to be a duplicate of an existing function in pkg/query-service/app/traces/v3/query_builder.go. Consider refactoring to use the existing function or extending it to cover this use case.

  • function enrichKeyWithMetadata (query_builder.go)

  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_0odxnd75cLc888mv


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

pkg/query-service/app/traces/v4/enrich.go Show resolved Hide resolved
@srikanthccv
Copy link
Member

@nityanandagohain, we should have the table schema-based logic in the traces as well. For example, if I create a resource_string_service$$namepace (doesn't matter how it's created assume it is created) then service.namespace should automatically routed to the column on the table. I don't see it in this PR.

@nityanandagohain
Copy link
Member Author

nityanandagohain commented Nov 15, 2024

It will be clear when I create a further PR (integration), but it's already in place.

If i have a key named service.namespace in resource attributes and there is a mat column resource_string_service$$namesapce, then the keys map will contains

{"service.namespace##resource##string": {Key: "service.namespace", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeResource, IsColumn: true}}

And the resource query builder wich is already integrated will take care of it.

@srikanthccv
Copy link
Member

If i have a key named service.namespace in resource attributes and there is a mat column resource_string_service$$namesapce, then the keys map will contains

Where exactly does this happen? How does it know there is a mat column resource_string_service$$namesapce?

@nityanandagohain
Copy link
Member Author

If you see the logs existing code, there is a function named GetLogFields which we call before enrichment of logs . There we get all the fields which have a corresponding materialized column. So this enrichment functions will already have the context if there exists a materialized column for corresponding attribute. That's what we pass to the Enrich() function as second parameter.

@srikanthccv
Copy link
Member

In case my question is not clear, I am asking where is it done for traces #6438 (comment)?

@nityanandagohain
Copy link
Member Author

nityanandagohain commented Nov 15, 2024

As I mentioned, it will be added in further PR's that integrates this Enrichment function. And there the logic for getting the keys which is a parameter for this enrichment function will be added.

This PR takes care that, given I have the keys map which contains all the conext of attributes and resource attributes, how do I enrich the user query.

@srikanthccv
Copy link
Member

bear with me; please help me understand the breakdown. What is done in this PR, and what is coming in the next set of PRs and what are those PRs? I could neither understand any of that from the PR description nor is it highlighted in the linked issue.

@nityanandagohain
Copy link
Member Author

have updated the description with the next set of things that are coming.

@srikanthccv
Copy link
Member

Please fix the build-pipeline

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.

❌ Changes requested. Incremental review on 87a4b97 in 18 seconds

More details
  • Looked at 27 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. pkg/query-service/utils/logs_test.go:100
  • Draft comment:
				t.Errorf("GenerateEnrichmentKeys() = %v, want %v", got, tt.want)
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_OPzmgoH7axeQcSyu


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

pkg/query-service/utils/logs_test.go Outdated Show resolved Hide resolved
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@nityanandagohain nityanandagohain merged commit 0acf39a into develop Nov 16, 2024
16 of 17 checks passed
@nityanandagohain nityanandagohain deleted the feat/trace-v4-enrichment branch November 16, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants