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

Deliver instrumentation data directly to amplitude, fixes #4335 #4866

Merged
merged 39 commits into from
Jun 21, 2023

Conversation

gilbertsoft
Copy link
Member

@gilbertsoft gilbertsoft commented May 3, 2023

The Issue

How This PR Solves The Issue

Statistics are now directly sent to Amplitude. All events are now properly logged again and to not to affect the user the data is locally cached and sent once the configurable limits are reached. The existing implementation using segment.io is still in place and removed in a second step when the new implementation is tested in the field and confirmed to work fine and like expected.

Manual Testing Instructions

To build the binary including the Amplitude API key use the following command:

AmplitudeAPIKey=xxx && make build where xxx has to be replaced by a valid key which can be found at https://data.amplitude.com/ddev/DDEV/sources/name/ddev

How to debug the data is explained at https://www.docs.developers.amplitude.com/data/debugger/.

It's also possible to create new charts with the data to debug it.

Settings DDEV_DEBUG=true and DDEV_VERBOSE=true will generate some additional output too which helps in testing.

There are also two new command ddev debug instrumentation flush to send all cached data to Amplitude and ddev debug instrumentation clean which will remove cached data. Helps also while testing.

Manual mass testing can be done with the following line:

export DDEV_VERBOSE=true; for ((i=1;i<=50;i++)); do ddev describe >/dev/null; done 2>&1 | tee ~/tmp/describe.out

This should do multiple submissions depending on the number of projects on the system.

Submitted events are visible at https://analytics.amplitude.com/ddev/activity afterwards.

This patch uses our forked repo of https://github.com/amplitude/analytics-go until the issue gets fixed in the upstream.

Automated Testing Overview

Most parts are covered by unit tests. Currently no real E2E tests are implemented and does look superfluous because no problems occured so far with the delivery and some parts are already tested by the upstream.

  • some tests are currently really minimal e.g. for the Tracker functions. Check if better testing is possible.

Related Issue Link(s)

Release/Deployment Notes

@github-actions
Copy link

github-actions bot commented May 3, 2023

@rfay
Copy link
Member

rfay commented May 3, 2023

Please make sure to explain what environment variables have to be set to make this work. I assume there's a secret that has to be compiled in or something. But we'll want to manually test here of course.

@rfay
Copy link
Member

rfay commented May 3, 2023

It looks like something important got lost: "no Go files in /home/runner/work/ddev/ddev/pkg/amplitude"

@rfay rfay changed the title Deliver instrumentation data directly to amplitude Deliver instrumentation data directly to amplitude, fixes #4335 May 3, 2023
@rfay
Copy link
Member

rfay commented May 5, 2023

The problem is that make test fails. pkg/amplitude has no go files, but pkg/amplitude/storages has one. I think it's just that the test process adds each directory in pkg to the list of items to test.

Adding pkg/amplitude/junk.go with contents

package amplitude

gets make test going. But it does seem to me like the structure here is odd. Probably just moving things around a bit will solve it. Just move the storage stuff into pkg/ampli? Or move the pkg/ampli stuff into pkg/amplitude?

@gilbertsoft
Copy link
Member Author

The problem is that make test fails. pkg/amplitude has no go files, but pkg/amplitude/storages has one. I think it's just that the test process adds each directory in pkg to the list of items to test.

Adding pkg/amplitude/junk.go with contents

package amplitude

gets make test going. But it does seem to me like the structure here is odd. Probably just moving things around a bit will solve it. Just move the storage stuff into pkg/ampli? Or move the pkg/ampli stuff into pkg/amplitude?

Thanks for the hint. Will check how I solve it, moving is not longer an option as I've added another package in the meantime.

@gilbertsoft gilbertsoft force-pushed the task/amplitude branch 5 times, most recently from 378b17f to 75a51e6 Compare May 7, 2023 20:52
@gilbertsoft gilbertsoft marked this pull request as ready for review May 8, 2023 12:18
@gilbertsoft gilbertsoft requested review from a team as code owners May 8, 2023 12:18
docs/content/developers/building-contributing.md Outdated Show resolved Hide resolved
docs/content/developers/building-contributing.md Outdated Show resolved Hide resolved
docs/content/developers/building-contributing.md Outdated Show resolved Hide resolved
@gilbertsoft gilbertsoft requested a review from mattstein May 9, 2023 09:31
@rfay
Copy link
Member

rfay commented May 10, 2023

Could you please analyze https://community.amplitude.com/announcements-56/updates-to-our-free-starter-plan-1819 and verify that it's not likely to affect us @gilbertsoft ? I think if users are defined in a way that works we should be OK?

@gilbertsoft
Copy link
Member Author

Could you please analyze https://community.amplitude.com/announcements-56/updates-to-our-free-starter-plan-1819 and verify that it's not likely to affect us @gilbertsoft ? I think if users are defined in a way that works we should be OK?

Regarding https://amplitude.com/pricing it's up to 100k users a month. At the moment we should not be affected though, bit more than 10k users a month.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Here's my first casual look-through, but I need more help to be able to manually test this. Please add manual testing steps, explaining how to build, and especially how to test, and show where to get the secret key. (I think you told me about the secret key somewhere in Discord, but it needs to be here.)

In Segment there was a nice debugging page where you could see events coming in. Is there something like that for Amplitude?

cmd/ddev/cmd/root.go Outdated Show resolved Hide resolved
docs/content/developers/release-management.md Outdated Show resolved Hide resolved
pkg/ddevapp/amplitude.go Outdated Show resolved Hide resolved
pkg/globalconfig/global_config.go Show resolved Hide resolved
third_party/ampli/ampli.go Show resolved Hide resolved
@gilbertsoft
Copy link
Member Author

gilbertsoft commented May 10, 2023

Here's my first casual look-through, but I need more help to be able to manually test this. Please add manual testing steps, explaining how to build, and especially how to test, and show where to get the secret key. (I think you told me about the secret key somewhere in Discord, but it needs to be here.)

In Segment there was a nice debugging page where you could see events coming in. Is there something like that for Amplitude?

Done

Btw I did not use the debugging tools so far, just played around with new charts and if events and properties are received is also visible on the data page at https://data.amplitude.com/ddev/DDEV/events/ampli/latest?view=All

@rfay
Copy link
Member

rfay commented May 11, 2023

I didn't get any extra output from ddev restart when I set DDEV_DEBUG=true or DDEV_VERBOSE=true

@gilbertsoft
Copy link
Member Author

I didn't get any extra output from ddev restart when I set DDEV_DEBUG=true or DDEV_VERBOSE=true

Works here

@gilbertsoft gilbertsoft force-pushed the task/amplitude branch 2 times, most recently from 216998c to fae1d97 Compare May 16, 2023 17:06
@rfay
Copy link
Member

rfay commented Jun 18, 2023

With export DDEV_VERBOSE=true; for ((i=1;i<=200;i++)); do ddev describe >/dev/null; done 2>&1 | tee ~/tmp/describe.out on e2556b7 (v1.22.0-alpha2-133-ge2556b775) I see the data coming into Amplitude, but the verbose output showing (anything) seems to be gone - no reports about the payload, or anything else. Now all I have is PERF: and such.

Edit: I think maybe you moved some of this to DDEV_DEBUG? I see it all when I do ddev debug instrumentation flush with DDEV_DEBUG=true; still don't see it when running the for loop.

Edit: Oh, you turned off reporting about many things? https://github.com/ddev/ddev/pull/4866/files#diff-251397554946dd8f37a081ff0d8638b12916434618051440c546fa08c20cf1bcR91

describe.txt
flush.txt

@rfay
Copy link
Member

rfay commented Jun 18, 2023

I think it's probably important to update the manual testing instructions with a more significant set of techniques. Please add quick and easy ways to test manually that definitely show the results. For example, some replacement for my for loop, how to look at amplitude, how to capture the output, etc.

A script that one could run that would manually verify would be great.

@gilbertsoft
Copy link
Member Author

gilbertsoft commented Jun 19, 2023

With export DDEV_VERBOSE=true; for ((i=1;i<=200;i++)); do ddev describe >/dev/null; done 2>&1 | tee ~/tmp/describe.out on e2556b7 (v1.22.0-alpha2-133-ge2556b775) I see the data coming into Amplitude, but the verbose output showing (anything) seems to be gone - no reports about the payload, or anything else. Now all I have is PERF: and such.

Edit: I think maybe you moved some of this to DDEV_DEBUG? I see it all when I do ddev debug instrumentation flush with DDEV_DEBUG=true; still don't see it when running the for loop.

It was always printed with DDEV_DEBUG=true and should still be the case, I did not change anything here. Maybe it's related to the redirect to /dev/null?

Edit: Oh, you turned off reporting about many things? https://github.com/ddev/ddev/pull/4866/files#diff-251397554946dd8f37a081ff0d8638b12916434618051440c546fa08c20cf1bcR91

describe.txt flush.txt

This is related to the old Segment instrumentation.

@gilbertsoft
Copy link
Member Author

I think it's probably important to update the manual testing instructions with a more significant set of techniques. Please add quick and easy ways to test manually that definitely show the results. For example, some replacement for my for loop, how to look at amplitude, how to capture the output, etc.

A script that one could run that would manually verify would be great.

Added an example of mass testing and where to see the incoming data on Amplitude.

Also changed the output behavior in the meantime, output releated to debugging should be printed to stderr now.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

OK, are you exhausted yet? I am!

Go ahead and pull this when you're ready.

@@ -0,0 +1,15 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I could only wish this was a jsonc and could have a comment explaining what it is and what it's for. Is it documented somewhere? Does it need to be in the root of the repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is created and managed by the ampli cli. We never should edit this file. Maybe I also could be placed in a subfolder but then you have to change to this directory to run the ampli cli which isn't nice imho.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just try to keep random files out of the root. Certainly not successful all the time.

@@ -0,0 +1,22 @@
package cmd
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought looking at these ddev debug instrumentation tools; Another one that did a test and showed whether data was being sent would be a win. But maybe the flush is good enough for that. But we'll be suspicious of this from time to time and it would be nice to have a super-quick test to see if it's working, or to ask people to use.

@gilbertsoft gilbertsoft merged commit adb7deb into ddev:master Jun 21, 2023
20 checks passed
@gilbertsoft gilbertsoft deleted the task/amplitude branch June 21, 2023 13:45
@rfay
Copy link
Member

rfay commented Jun 21, 2023

Yippee! Congratulations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants