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

Auth: Add anonymous users view and stats #78685

Merged
merged 11 commits into from
Nov 29, 2023

Conversation

eleijonmarck
Copy link
Contributor

@eleijonmarck eleijonmarck commented Nov 27, 2023

What is this feature?

  • anonymous users users page
  • add feature toggle displayAnonymousStats
  • remove check for enterprise for X-Grafana-Device-Id header in request
  • add anonusers/device count to stats

Why do we need this feature?
We want to surface the anonymous users to our users as well as show a count of them in the stats page.

Special notes for your reviewer:
image
image

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Copy link
Contributor

Hello @eleijonmarck!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@eleijonmarck eleijonmarck added product-approved Pull requests that are approved by product/managers and are allowed to be backported and removed product-approved Pull requests that are approved by product/managers and are allowed to be backported labels Nov 27, 2023
@eleijonmarck eleijonmarck marked this pull request as ready for review November 27, 2023 12:44
@eleijonmarck eleijonmarck requested review from grafanabot and a team as code owners November 27, 2023 12:44
@eleijonmarck eleijonmarck requested review from tskarhed, Clarity-89, mildwonkey, nikimanoledaki, suntala and Jguer and removed request for a team November 27, 2023 12:44
@eleijonmarck eleijonmarck added the product-approved Pull requests that are approved by product/managers and are allowed to be backported label Nov 27, 2023
- anonymous users users page
- add feature toggle `anonymousAccess`
- remove check for enterprise for `Device-Id` header in request
- add anonusers/device count to stats
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/anon-access/anonymous-users branch from d85bbec to f6575eb Compare November 27, 2023 13:22
pkg/services/anonymous/anonimpl/api/api.go Outdated Show resolved Hide resolved
pkg/services/anonymous/anonimpl/api/api.go Outdated Show resolved Hide resolved
pkg/api/dtos/frontend_settings.go Outdated Show resolved Hide resolved
pkg/services/anonymous/service.go Outdated Show resolved Hide resolved
Copy link
Contributor

@IevaVasiljeva IevaVasiljeva left a comment

Choose a reason for hiding this comment

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

Great to see progress on this! Adding some comments that I had after a quick skim through the PR.

pkg/services/anonymous/anonimpl/api/api.go Outdated Show resolved Hide resolved
pkg/services/anonymous/anonimpl/api/api.go Outdated Show resolved Hide resolved
public/app/features/admin/state/reducers.ts Outdated Show resolved Hide resolved
pkg/services/anonymous/anonimpl/api/api.go Show resolved Hide resolved
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/anon-access/anonymous-users branch from c8f314e to 5f97916 Compare November 27, 2023 15:27
public/app/features/admin/ServerStats.tsx Outdated Show resolved Hide resolved
public/app/features/admin/UserListAnonymousPage.tsx Outdated Show resolved Hide resolved
public/app/features/admin/UserListAnonymousPage.tsx Outdated Show resolved Hide resolved
public/app/features/admin/UserListAnonymousPage.tsx Outdated Show resolved Hide resolved
public/app/types/user.ts Outdated Show resolved Hide resolved
public/app/features/admin/Users/AnonUsersTable.tsx Outdated Show resolved Hide resolved
public/app/features/admin/state/apis.tsx Outdated Show resolved Hide resolved
public/app/features/admin/state/reducers.ts Outdated Show resolved Hide resolved
public/app/features/admin/state/reducers.ts Outdated Show resolved Hide resolved
pkg/services/anonymous/anonimpl/api/api.go Show resolved Hide resolved
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/anon-access/anonymous-users branch from 6ec1e17 to dd34d74 Compare November 28, 2023 10:57
pkg/api/admin.go Outdated Show resolved Hide resolved
eleijonmarck and others added 2 commits November 28, 2023 14:14
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Copy link
Contributor

@Clarity-89 Clarity-89 left a comment

Choose a reason for hiding this comment

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

Frontend changes look good!

public/app/core/services/backend_srv.ts Show resolved Hide resolved
public/app/features/admin/ServerStats.tsx Outdated Show resolved Hide resolved
public/app/features/admin/ServerStats.tsx Show resolved Hide resolved
@Jguer Jguer merged commit 59bdff0 into main Nov 29, 2023
21 checks passed
@Jguer Jguer deleted the eleijonmarck/anon-access/anonymous-users branch November 29, 2023 16:58
Copy link
Contributor

The backport to v10.2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-78685-to-v10.2.x origin/v10.2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 59bdff0280d52ca5d8918157d7697b9279b25501

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-78685-to-v10.2.x
# Create the PR body template
PR_BODY=$(gh pr view 78685 --json body --template 'Backport 59bdff0280d52ca5d8918157d7697b9279b25501 from #78685{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[v10.2.x] Auth: Add anonymous users view and stats" --body-file - --label "type/docs" --label "area/backend" --label "area/frontend" --label "add to changelog" --label "product-approved" --label "backport" --base v10.2.x --milestone 10.2.x --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-78685-to-v10.2.x

# Create a pull request where the `base` branch is `v10.2.x` and the `compare`/`head` branch is `backport-78685-to-v10.2.x`.

# Remove the local backport branch
git switch main
git branch -D backport-78685-to-v10.2.x

@grafana-delivery-bot grafana-delivery-bot bot added the backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. label Nov 29, 2023
Jguer pushed a commit that referenced this pull request Dec 1, 2023
* Add anonymous stats and user table

- anonymous users users page
- add feature toggle `anonymousAccess`
- remove check for enterprise for `Device-Id` header in request
- add anonusers/device count to stats

* promise all, review comments

* make use of promise all settled

* refactoring: devices instead of users

* review comments, moved countdevices to httpserver

* fakeAnonService for tests and generate openapi spec

* do not commit openapi3 and api-merged

* add openapi

* Apply suggestions from code review

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* formatin

* precise anon devices to avoid confusion

---------

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Co-authored-by: jguer <me@jguer.space>
(cherry picked from commit 59bdff0)
Jguer added a commit that referenced this pull request Dec 4, 2023
* Auth: Add anonymous users view and stats (#78685)

* Add anonymous stats and user table

- anonymous users users page
- add feature toggle `anonymousAccess`
- remove check for enterprise for `Device-Id` header in request
- add anonusers/device count to stats

* promise all, review comments

* make use of promise all settled

* refactoring: devices instead of users

* review comments, moved countdevices to httpserver

* fakeAnonService for tests and generate openapi spec

* do not commit openapi3 and api-merged

* add openapi

* Apply suggestions from code review

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* formatin

* precise anon devices to avoid confusion

---------

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Co-authored-by: jguer <me@jguer.space>
(cherry picked from commit 59bdff0)

* update component to experimental

---------

Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>
...(config.featureToggles.displayAnonymousStats && stats.activeDevices
? [
{ name: 'Active anonymous devices in last 30 days', value: stats.activeDevices },
{ name: 'Active anonymous users in last 30 days', value: Math.floor(stats.activeDevices / 3) },
Copy link
Contributor

Choose a reason for hiding this comment

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

@eleijonmarck i know this is behind a feature toggle and stuff, but is this intentional? doesn't feel like the number of active users will be numActiveDevices / 3 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashharrison90 yea this is intentional. We count the number of users based on devices is activeDevices/3

I can send over the design doc from product. This is products decision

Copy link
Contributor

Choose a reason for hiding this comment

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

...ok 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

@eleijonmarck I can see how it's pretty confusing though. Maybe we can add a comment explaining the thinking (in one of the future PRs)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend area/frontend backport v10.2.x backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. product-approved Pull requests that are approved by product/managers and are allowed to be backported type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants