-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: adds trace funnels #7230
base: main
Are you sure you want to change the base?
feat: adds trace funnels #7230
Conversation
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
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.
❌ Changes requested. Reviewed everything up to 2e72592 in 3 minutes and 14 seconds
More details
- Looked at
629
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. pkg/query-service/app/integrations/trace-funnels/Readme.md:13
- Draft comment:
Fix typos: change 'papulate' to 'populate' and 'retreive' (line 17) to 'retrieve'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a newly added Readme that appears to be a working document/scratch pad, given the "rough work" header and informal bullet-point style. While the typos are real, this doesn't seem like production documentation where perfect spelling would be critical. The comment is technically about changes since this is a new file, but it feels overly pedantic for a rough work document.
The typos could propagate into actual code or documentation later if not fixed now. Clear communication is important even in planning documents.
While clear communication is important, this appears to be a temporary planning document and the meaning is still clear despite the typos. The effort of fixing these minor typos outweighs the benefit.
Delete the comment as it's overly pedantic for a rough work document where perfect spelling isn't critical to understanding or functionality.
2. pkg/query-service/app/integrations/trace-funnels/funnels_test.go:319
- Draft comment:
Avoid sharing a single storage instance across multiple tests to prevent state leakage. Consider initializing storage in each test case. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
Looking at the test cases, they are intentionally designed to be sequential and dependent - creating a funnel, then getting it, updating it, listing it, and deleting it. The shared storage is part of the test design to verify the full lifecycle. Moving storage initialization into each test case would break this intended behavior. The comment misunderstands the test's purpose.
While test isolation is generally good practice, there are valid cases for intentionally shared state in tests, especially when testing stateful operations in sequence. Am I being too dismissive of the isolation concern?
No - the test is explicitly designed to verify storage operations in sequence. The shared state is not an accident but a requirement for the test scenario. The other test functions (TestFunnelOperations and TestFunnelStepManagement) properly initialize fresh storage for each test case.
The comment should be deleted as it misunderstands the intentional design of the storage operation tests which require shared state to verify the full lifecycle of funnel operations.
3. pkg/query-service/app/integrations/trace-funnels/Readme.md:17
- Draft comment:
Typo: 'retreive' should be 'retrieve'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a new file, so it is about changes. The spelling error exists. However, this appears to be a rough work/notes file based on the "// rough work" comment at the top and the informal bullet-point style. Spelling corrections in rough documentation don't add significant value and could be considered nitpicking.
The spelling error could propagate to actual code or documentation if not caught early. Documentation quality matters even in rough notes.
Given this is explicitly marked as "rough work" and contains other informal notes, this level of nitpicking isn't valuable and could create unnecessary noise in the PR review.
Delete this comment as it's an overly pedantic correction in a rough work document that doesn't impact functionality or understanding.
4. pkg/query-service/app/integrations/trace-funnels/Readme.md:24
- Draft comment:
Typo: Replace 'receving' with 'receiving'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a new file being added. The typo is real, but this appears to be a rough work/documentation file with multiple other typos ('papulate', 'retreive'). The comment is about a change since this is a new file. However, commenting on typos in what appears to be temporary/draft documentation feels overly pedantic and not valuable.
The typo could propagate if this documentation becomes permanent. Clear communication is important even in draft documents.
The file is clearly marked as "rough work" and contains multiple informal notes and TODOs. This level of nitpicking on draft documentation creates noise rather than value.
Delete the comment as it's overly pedantic for a rough work document and doesn't add meaningful value to the PR review.
5. pkg/query-service/app/integrations/trace-funnels/funnels.go:30
- Draft comment:
Possible typo in comment: 'ToDp:' likely intended as 'ToDo:'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the typo observation is correct, fixing typos in comments is a very minor issue. The meaning is still clear despite the typo. The rules state not to make comments that are obvious or unimportant. A typo in a TODO comment is not something that impacts functionality or code quality.
The typo could be indicative of overall lack of attention to detail. Multiple TODO comments might suggest incomplete implementation.
The rules explicitly state not to make comments that are obvious or unimportant. A simple typo in a comment falls into this category.
This comment should be deleted as it points out a trivial typo that doesn't impact code functionality or quality.
6. pkg/query-service/app/integrations/trace-funnels/storage.go:12
- Draft comment:
Interface signature mismatch: FunnelStorage methods expect 'Funnel' by value, but implementations use '*Funnel'. Align these types. - Reason this comment was not posted:
Marked as duplicate.
7. pkg/query-service/app/integrations/trace-funnels/Readme.md:13
- Draft comment:
Typo found: 'papulate' should be corrected to 'populate'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a documentation file that appears to be rough work/notes. The typo is minor and doesn't affect functionality. The file itself says "rough work" at the top. Documentation typos, especially in rough work files, are not critical enough to warrant a PR comment according to our rules about only commenting on clearly required code changes.
The typo could potentially cause confusion for other developers reading the documentation. Documentation quality is important for maintainability.
While documentation quality matters, this is explicitly marked as "rough work" and the typo is minor enough that any reader would understand the intended meaning. This doesn't meet the threshold of "clearly a code change required."
Delete the comment as it violates the rule about only commenting when there is clearly a code change required. Minor documentation typos in rough work files don't meet this threshold.
8. pkg/query-service/app/integrations/trace-funnels/Readme.md:17
- Draft comment:
Typo found: 'retreive' should be corrected to 'retrieve'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This is a rough work/planning document, not production code or user-facing documentation. The typo doesn't impact functionality or understanding. The document contains other informal elements and even other typos ('papulate', 'receving') that weren't commented on. Making spelling corrections in informal planning docs creates unnecessary noise.
The typo could propagate to actual code or documentation later. Clear communication is important even in planning documents.
This is clearly marked as "rough work" and contains multiple informal elements. The benefit of fixing this typo doesn't outweigh the noise it adds to the PR review.
Delete the comment as it's an unimportant spelling correction in an informal planning document that doesn't impact functionality or understanding.
9. pkg/query-service/app/integrations/trace-funnels/Readme.md:24
- Draft comment:
Typo found: 'receving' should be corrected to 'receiving'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a newly added README that appears to be a working document/scratch pad. It contains multiple typos and informal notes. The comment is technically correct about the typo, but this doesn't seem like the type of document where spelling precision matters. It's marked as "rough work" at the top.
The typo could cause confusion if this document becomes permanent documentation. Also, fixing typos early prevents them from being copied elsewhere.
This is explicitly marked as "rough work" and contains multiple informal notes and typos. The value of fixing a single typo in such a document is minimal.
Delete the comment. Nitpicking typos in an informal working document doesn't add value and goes against the rule of not making purely informative comments.
10. pkg/query-service/app/integrations/trace-funnels/funnels.go:30
- Draft comment:
Typographical error found in the comment on line 30. Consider changing 'ToDp:' to 'TODO:' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct about the typo, TODO comment formatting is a minor issue that doesn't affect functionality. The comment doesn't suggest any actual code changes that would improve the quality or functionality of the code. It's more of a stylistic/formatting issue that could be handled by a linter or automated tool.
The inconsistent TODO formatting across the file could indicate a broader style guide issue that should be addressed. The typo could cause confusion when searching for TODOs.
While consistency is good, this kind of minor formatting issue doesn't warrant a PR comment. It would be better handled by team style guidelines or automated tools.
Delete the comment as it focuses on a minor formatting issue rather than substantive code changes or improvements.
11. pkg/query-service/app/integrations/trace-funnels/storage.go:78
- Draft comment:
Minor typographical suggestion: Consider replacing 'ToDo:' with 'TODO:' in the comment on line 78 to adhere to common convention. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While "TODO:" is indeed more conventional than "ToDo:", this is an extremely minor stylistic issue that doesn't affect functionality. The meaning is perfectly clear either way. According to the rules, we should not make comments that are obvious or unimportant. This feels like unnecessary nitpicking.
The comment does point out a real inconsistency with common Go conventions. Some teams might have strict style guides about this.
Even if it's technically correct, this kind of minor style nitpick creates noise and doesn't meaningfully improve code quality. The rules explicitly say to avoid obvious/unimportant comments.
Delete this comment as it's too minor and doesn't provide meaningful value. This kind of style nitpick should be handled by automated linting if the team cares about it.
Workflow ID: wflow_UWjmVWRkwJcDfDXi
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.
|
||
// NewFunnel creates a new funnel with default values | ||
|
||
// ToDp: use this funnel to create a new funnel with unique name, return error with the funnel with same name exists |
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.
Typo in comment: 'ToDp:'
should be 'TODO:'
. Also consider if DeleteStep
should update orders of subsequent steps.
// ToDp: use this funnel to create a new funnel with unique name, return error with the funnel with same name exists | |
// TODO: use this funnel to create a new funnel with unique name, return error with the funnel with same name exists |
// FunnelStorage defines the interface for storing and retrieving funnels | ||
|
||
type FunnelStorage interface { | ||
CreateFunnel(ctx context.Context, funnel Funnel) error |
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.
Interface signature mismatch: FunnelStorage
interface uses value (Funnel
) but implementations use pointer (*Funnel
). Use pointers consistently.
CreateFunnel(ctx context.Context, funnel Funnel) error | |
CreateFunnel(ctx context.Context, funnel *Funnel) error |
|
||
// write main function to | ||
|
||
// create and papulate funnels |
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.
Typo: Replace 'papulate' with 'populate'.
// create and papulate funnels | |
// create and populate funnels |
func (f *Funnel) UpdateStep(order int64, newStep FunnelStep) error { | ||
for i, step := range f.Steps { | ||
if step.Order == order { | ||
f.Steps[i] = newStep |
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.
Consider preserving the original step order in UpdateStep
(e.g., assign newStep.Order = order
) to avoid losing ordering info.
f.Steps[i] = newStep | |
newStep.Order = order; f.Steps[i] = newStep |
|
||
// InMemoryFunnelStorage is a simple in-memory implementation for v0 | ||
type InMemoryFunnelStorage struct { | ||
funnels map[string]*Funnel |
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.
InMemoryFunnelStorage
's map is not concurrency-safe; consider adding synchronization (e.g., using sync.Mutex
) if accessed concurrently.
related to #7210
Important
Adds trace funnels feature with funnel management and in-memory storage, including tests for CRUD operations.
trace funnels
for managing trace data infunnels.go
.Funnel
andFunnelStep
structs for funnel management.NewFunnel()
,AddStep()
,UpdateStep()
,DeleteStep()
for funnel operations infunnels.go
.storage.go
withInMemoryFunnelStorage
.TestFunnelOperations()
,TestFunnelStepManagement()
,TestFunnelStorageOperations()
infunnels_test.go
to validate funnel creation, modification, and storage operations.Readme.md
with initial notes on trace funnels.This description was created by
for 2e72592. It will automatically update as commits are pushed.