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

chore: add identify and group event support to /event API #7219

Merged
merged 4 commits into from
Mar 6, 2025

Conversation

makeavish
Copy link
Member

@makeavish makeavish commented Mar 5, 2025

Summary

add identify and group event support to /event API


Important

Add support for identify and group events to /event API, including validation and telemetry handling.

  • Behavior:
    • Add support for IdentifyEvent and GroupEvent in registerEvent() in http_handler.go.
    • SendIdentifyEvent() and SendGroupEvent() added to telemetry.go to handle new event types.
  • Validation:
    • parseRegisterEventRequest() in parser.go updated to validate EventType and EventName for TrackEvent.
  • Models:
    • Add EventType and RegisterEventParams to queryParams.go to define new event types and parameters.

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

@makeavish makeavish requested a review from prashant-shahi March 5, 2025 02:34
@github-actions github-actions bot added the chore label Mar 5, 2025
@makeavish makeavish marked this pull request as ready for review March 6, 2025 13:15
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 0a5e340 in 2 minutes and 13 seconds

More details
  • Looked at 203 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. pkg/query-service/telemetry/telemetry.go:111
  • Draft comment:
    Use idiomatic camelCase for variable names. For example, rename 'random_number' to 'randomNumber' for clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/telemetry/telemetry.go:705
  • Draft comment:
    The call to ignoreEvents(event, data) appears without a definition in this file. Ensure it’s defined and well documented to explain its criteria for dropping events.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/telemetry/telemetry.go:262
  • Draft comment:
    Setting telemetry.maxRandInt using int(1/DEFAULT_SAMPLING) works for DEFAULT_SAMPLING=0.1, but consider guarding against changes to DEFAULT_SAMPLING to avoid division‐by‐zero or unexpected rounding.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/query-service/telemetry/telemetry.go:662
  • Draft comment:
    BUG: In SetCompanyDomain, the if statement does not guard against accessing email_split[1]. If len(email_split) != 2, this will panic. Use an else clause to set the domain only when the split has exactly 2 parts.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/telemetry/telemetry.go:98
  • Draft comment:
    SECURITY: Avoid hardcoding API keys (e.g. the 'api_key' constant). Use environment variables or a secure configuration mechanism.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/telemetry/telemetry.go:529
  • Draft comment:
    IMPROVEMENT: The getOutboundIP function uses http.Get without a timeout. Consider using an HTTP client with a timeout to avoid potential blocking if the external service is slow or unreachable.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pkg/query-service/telemetry/telemetry.go:705
  • Draft comment:
    QUESTION: The SendEvent function calls an undefined function 'ignoreEvents'. Ensure that 'ignoreEvents' is defined or imported, or remove this check if not needed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/telemetry/telemetry.go:710
  • Draft comment:
    SUGGESTION: In SendEvent, the rate limiting logic uses a mutex and a map. For high-concurrency scenarios, consider using atomic counters to improve performance.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/app/http_handler.go:4930
  • Draft comment:
    Typo: The variable name 'errQuriesByName' (and similar variants like 'errQuriesByNameFetchLatency') is misspelled. It should be 'errQueriesByName'. Please update the spelling for consistency and clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/app/http_handler.go:4984
  • Draft comment:
    Typo in comment: 'experssion' should be 'expression'. Please correct the spelling in the comment for clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_sJA1Rxe9GMIKtxRd


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

@makeavish makeavish requested a review from vikrantgupta25 March 6, 2025 18:33
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 094da5e in 1 minute and 44 seconds

More details
  • Looked at 53 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. pkg/query-service/app/parser.go:39
  • Draft comment:
    The variable 'allowedFunctions' is declared but never used. Remove or integrate it to avoid dead code.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pkg/query-service/app/parser.go:387
  • Draft comment:
    The hardcoded buffer value (30000000000 ns) in parseTimeMinusBufferStr is a magic number. Consider replacing it with a named constant and comment its purpose.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/query-service/app/parser.go:71
  • Draft comment:
    In parseRegisterEventRequest, the error message for an invalid eventType is generic. Consider listing the valid options (track, identify, group) in the error for better clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. pkg/query-service/app/parser.go:808
  • Draft comment:
    validateExpressions compares each variable (from govaluate) to a builder query's full expression. This may be overly strict if variables are meant to be referenced by query identifier rather than the whole expression. Verify and consider matching against a dedicated query name.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/query-service/app/parser.go:358
  • Draft comment:
    There is an inconsistency between time parsers: parseMetricsTime converts a float (treating s as seconds with fraction) while parseTimeStr (and parseTime) expect a string representing an integer (nanoseconds). Ensure the units are consistent or add documentation to clarify the expected input for each.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pkg/query-service/model/queryParams.go:53
  • Draft comment:
    New event support: The additional event types (identify, group) in EventType and RegisterEventParams look correct. Ensure that downstream telemetry processing properly handles these new events.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
7. pkg/query-service/app/http_handler.go:3089
  • Draft comment:
    The variable name 'errQuriesByName' appears in several places. It looks like a typographical error—likely it should be spelled 'errQueriesByName'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pkg/query-service/app/http_handler.go:4566
  • Draft comment:
    The variable 'lastestConfig' appears to be misspelled. It is likely intended to be 'latestConfig'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/app/http_handler.go:4957
  • Draft comment:
    There is a typographical error in the comment: 'experssion' should be 'expressions'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/app/parser.go:1041
  • Draft comment:
    Typographical issue: The variable name 'thirdPartApis' in the ParseRequestBody function (around line 1041) appears to be a typo. It would be clearer and more consistent to rename it to 'thirdPartyApis'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_cJ2cJOXlg5k310Y5


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

@makeavish makeavish merged commit 819428a into main Mar 6, 2025
15 of 16 checks passed
@makeavish makeavish deleted the chore/add-identify-group-event-support branch March 6, 2025 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants