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

Identify Call Improvements #8422

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Identify Call Improvements #8422

merged 1 commit into from
Feb 24, 2022

Conversation

jakobhero
Copy link
Contributor

@jakobhero jakobhero commented Feb 23, 2022

Description

PR improves data quality of identify call by making the following two changes:

  • the IP is read directly from the xff header instead of the request middleware, which did not contain the client IP in the leftmost position described in the docs. the position of IPs in the xff header was verified through server logs
  • include the region as user trait. this enables to segment users on an additional granularity level between city and country

Related Issue(s)

How to test

The PR depends on http headers populated and modified by the load balancer, so it cannot be accurately tested in the preview environment. ensuring that none of the previous functionality breaks can be done through:

  • opening preview environment and signing up
  • opening the Segment's Staging Untrusted source and verifying that the identify events come through for signup and login. don't worry if the IP in the payload is nonsensical, this is to be expected in the preview environment

Release Notes

NONE

Documentation

/werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #8422 (a2ca2ce) into main (343ae26) will decrease coverage by 21.67%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #8422       +/-   ##
===========================================
- Coverage   32.85%   11.17%   -21.68%     
===========================================
  Files          33       18       -15     
  Lines        4745      993     -3752     
===========================================
- Hits         1559      111     -1448     
+ Misses       3068      880     -2188     
+ Partials      118        2      -116     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-ws-manager-app ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/ws-manager/pkg/manager/status.go
components/ws-manager/pkg/manager/create.go
components/local-app/pkg/auth/pkce.go
components/ws-manager/pkg/manager/manager.go
...s/ws-manager/pkg/manager/internal/grpcpool/pool.go
components/ws-manager/pkg/clock/clock.go
components/local-app/pkg/auth/auth.go
...omponents/ws-manager/pkg/manager/pod_controller.go
components/ws-manager/pkg/manager/probe.go
components/ws-manager/pkg/manager/metrics.go
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 343ae26...a2ca2ce. Read the comment docs.

@jakobhero jakobhero marked this pull request as ready for review February 23, 2022 20:44
@jakobhero jakobhero requested a review from a team February 23, 2022 20:44
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 23, 2022
Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

code changes look good to me.

@roboquat roboquat merged commit 1faa7f5 into main Feb 24, 2022
@roboquat roboquat deleted the jh/read-ip-from-xff-header branch February 24, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants