[POA-41] Update analytics functions to support amplitude functions #246
Conversation
mudit-postman
commented
Nov 15, 2023
•
mudit-postman
commented
- Replace Segment related keys and mentioned with Amplitude Keys and configs
- Updated few event names to make them consistent in Amplitude
| @@ -114,7 +114,7 @@ func getDistinctID() string { | |||
| // Otherwise use the configured API Key. | |||
| // Failing that, try to use the user name and host name? | |||
|
|
|||
| id := os.Getenv("AKITA_SEGMENT_DISTINCT_ID") | |||
| id := os.Getenv("POSTMAN_ANALYTICS_DISTINCT_ID") | |||
There was a problem hiding this comment.
One place is in the integration test L7
There was a problem hiding this comment.
No, this is not a documented feature. The ability to disable analytics is more important.
| @@ -24,7 +24,7 @@ var ( | |||
| analyticsEnabled bool | |||
|
|
|||
| // Client key; set at link-time with -X flag | |||
There was a problem hiding this comment.
Needs a corresponding change in superstar/cli-postman and superstar/cli-akita
| // IsMixpanelEnabled: false, -- irrelevant for us, leaving at default value | ||
| BatchSize: 1, // disable batching |
There was a problem hiding this comment.
I don't recall precisely why I disabled batching, but I think it had to do with catching errors during command-line execution, or maybe other abnormal exits. I think that should still be part of our use of Amplitude, if batching can be disabled?
There was a problem hiding this comment.
We can disable batching we can use FlushQueueSize config for this.
| @@ -114,7 +114,7 @@ func getDistinctID() string { | |||
| // Otherwise use the configured API Key. | |||
| // Failing that, try to use the user name and host name? | |||
|
|
|||
| id := os.Getenv("AKITA_SEGMENT_DISTINCT_ID") | |||
| id := os.Getenv("POSTMAN_ANALYTICS_DISTINCT_ID") | |||
There was a problem hiding this comment.
No, this is not a documented feature. The ability to disable analytics is more important.
| func Failure(message string) { | ||
| analyticsClient.Track(distinctID(), | ||
| message, | ||
| fmt.Sprintf("Unknown Error: %s", message), |
There was a problem hiding this comment.
I am unsure about the need to add the prefixes as you have done here. I don't think we really felt their absence using Mixpanel, and the "type" field could be used to filter. Is this an Amplitude or Postman best practice that we should understand?
We should not use "Unknown Error" here, though, because the failures can be known errors that simply don't come with a Go "error".
There was a problem hiding this comment.
I have just added these prefixes to follow a convention with the event names, also it can help us in the easier resolution of new events that might be added in the Amplitude.
Amplitude doesn't automatically start tracking a new event, we need to tell Amplitude to start tracking the event. Like in the attached image, events that are not being tracked are marked as Unexpected we need to add them to the list to make them live.
https://app.amplitude.com/data/postman/Postman/events/POA-41-migrate-to-amplitude/latest?view=All
PS: Events in image are before adding prefixes to event names
| @@ -268,7 +268,7 @@ func Failure(message string) { | |||
| // Report success of an operation | |||
| func Success(message string) { | |||
| analyticsClient.Track(distinctID(), | |||
| message, | |||
| fmt.Sprintf("Success in %s", message), | |||
There was a problem hiding this comment.
For example, in this case we have only two instances:
"Success in rotate learn session"
"Success in Add to ECS"
which is inconsistent capitalization :) and I don't think the success adds much?
|
Fixed |
