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

sql: change github links to go through a proxy to gather click telemetry #45504

Closed
jordanlewis opened this issue Feb 27, 2020 · 15 comments · Fixed by #49836
Closed

sql: change github links to go through a proxy to gather click telemetry #45504

jordanlewis opened this issue Feb 27, 2020 · 15 comments · Fixed by #49836
Assignees
Labels
A-telemetry C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@jordanlewis
Copy link
Member

Currently, many error messages in CockroachDB include a link to an issue on GitHub. We think (and hope) that many people click these links.

We should use a link shortener to proxy these links, so that we can gather click metrics (how many times a particular issue is clicked on). This will help prioritize issues further.

For example, the link could look like https://cockroa.ch/issue/34734, which could redirect to issue #34734 as a random example. Another benefit to this is that, if we ever change issue management technologies, this could be transparent to our users.

cc @bobvawter, @kenliu, @rafiss, @apantel, @awoods187

@awoods187
Copy link
Contributor

Love this idea!

@bobvawter
Copy link
Member

I can add pattern-redirects to go.crdb.dev (and certainly support multiple domain frontends).

@jordanlewis
Copy link
Member Author

@bobvawter I like the idea. The main downside to this idea is that we'd be committing to operating the proxy as a production service ~forever.

@bobvawter
Copy link
Member

It's not a proxy so much as a redirector. If we ever wanted to turn that service down, we could generate a whole slew of static html pages from the link database and just dump them into a public-facing GCP storage bucket.

@rafiss
Copy link
Collaborator

rafiss commented Feb 27, 2020

Perhaps for clusters that have telemetry enabled, we could also include a query param in the URL (e.g. ?cid=<uuid unique to cluster>). We could decide to have this be the same as the cluster ID reported to us in telemetry, or make it distinct.

The idea is then we could have additional data on how often each cluster/user runs into an unimplemented issue.

@awoods187
Copy link
Contributor

Have we had any more thoughts here?

@bobvawter
Copy link
Member

If we're comfortable using the go/ redirector to capture this, I'll add pattern redirects to it this Friday.

@awoods187
Copy link
Contributor

@apantel this is the issue

@kenliu
Copy link

kenliu commented May 18, 2020

Do we have the redirector behind IAP?

@bobvawter
Copy link
Member

The bulk of go/ lives behind IAP. There is a carve-out for go/p/... paths to allow some links to be published to the entire world.

@bobvawter
Copy link
Member

Just to be explicit, only links which have been marked as public are actually available on the /p/ prefix.

@bobvawter
Copy link
Member

@awoods187 What data do you want to record about an incoming redirect request? Do you want PII like IP address, User-Agent, query params? There's already a trivial click-counting table that I can add more columns to. This data could easily be synched out to Snowflake.

@bobvawter
Copy link
Member

Pattern redirects are working now:

https://go.crdb.dev/pr/1234
https://go.crdb.dev/issue/5678

These two path prefixes bypass the IAP proxy and could be used externally. Alternate domain names can be used without significant extra effort.

Let me know what data you want to capture for clicks.

@rafiss
Copy link
Collaborator

rafiss commented Jun 3, 2020

I think to start we'd just want to capture click counts.

Is it easy to capture more data? It doesn't seem worth spending much time on anything else, since I don't anticipate anyone having the time to dive very deep into the data here.

@bobvawter
Copy link
Member

bobvawter commented Jun 3, 2020 via email

@knz knz assigned rohany and rafiss Jun 4, 2020
@knz knz added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jun 4, 2020
@craig craig bot closed this as completed in 588c0a3 Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-telemetry C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants