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

Track new connection: #2260

Merged
merged 4 commits into from
Mar 5, 2021
Merged

Track new connection: #2260

merged 4 commits into from
Mar 5, 2021

Conversation

ChristopheDuong
Copy link
Contributor

What

Describe what the change is solving

Closes #2163

How

Describe the solution

@@ -95,6 +95,9 @@ public void track(String action, Map<String, Object> metadata) {
final Map<String, Object> mapCopy = new HashMap<>(metadata);
final TrackingIdentity trackingIdentity = identitySupplier.get();
mapCopy.put(AIRBYTE_VERSION_KEY, trackingIdentity.getAirbyteVersion());
if (metadata.containsKey(USER_EMAIL_KEY_PLACEHOLDER) && metadata.get(USER_EMAIL_KEY_PLACEHOLDER).equals(USER_EMAIL_VALUE_PLACEHOLDER)) {
Copy link
Contributor Author

@ChristopheDuong ChristopheDuong Mar 2, 2021

Choose a reason for hiding this comment

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

Option1: I put this little placeholder trick so we can tweak if we want to inject user email or not...

Option2: If we are okay with always sending user email for both "Connector Jobs" and "New Connection - Backend", then this is not needed...

Option3: This could also be solved with a new method in the interface TrackingClient? trackWithEmail?

(as commented here: #2163 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

@cgardens just want to make sure we are not storing the email if someone opted out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am in favor of option2 let's keeps things simple, we can always revisit if needed

Copy link
Contributor

@johnlafleur johnlafleur Mar 3, 2021

Choose a reason for hiding this comment

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

I'm ok with option 2. are you okay with it @cgardens ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. option 2 sounds good.

@@ -95,6 +95,9 @@ public void track(String action, Map<String, Object> metadata) {
final Map<String, Object> mapCopy = new HashMap<>(metadata);
final TrackingIdentity trackingIdentity = identitySupplier.get();
mapCopy.put(AIRBYTE_VERSION_KEY, trackingIdentity.getAirbyteVersion());
if (metadata.containsKey(USER_EMAIL_KEY_PLACEHOLDER) && metadata.get(USER_EMAIL_KEY_PLACEHOLDER).equals(USER_EMAIL_VALUE_PLACEHOLDER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@cgardens just want to make sure we are not storing the email if someone opted out.

@@ -95,6 +95,9 @@ public void track(String action, Map<String, Object> metadata) {
final Map<String, Object> mapCopy = new HashMap<>(metadata);
final TrackingIdentity trackingIdentity = identitySupplier.get();
mapCopy.put(AIRBYTE_VERSION_KEY, trackingIdentity.getAirbyteVersion());
if (metadata.containsKey(USER_EMAIL_KEY_PLACEHOLDER) && metadata.get(USER_EMAIL_KEY_PLACEHOLDER).equals(USER_EMAIL_VALUE_PLACEHOLDER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am in favor of option2 let's keeps things simple, we can always revisit if needed

Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

looks good assuming we go forward with option 2.

@ChristopheDuong
Copy link
Contributor Author

@cgardens just want to make sure we are not storing the email if someone opted out.

This check is made here:

if (workspace.getEmail() != null && workspace.getAnonymousDataCollection() != null && !workspace.getAnonymousDataCollection()) {

@ChristopheDuong ChristopheDuong merged commit 6c3378c into master Mar 5, 2021
@ChristopheDuong ChristopheDuong deleted the chris/track-new-connection branch March 5, 2021 13:50
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.

Log users who created a connection in Orbit, when not anonymized
4 participants