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

telemetry: add tag deployment_type #132

Merged
merged 3 commits into from
May 24, 2023

Conversation

juliangruber
Copy link
Member

@juliangruber juliangruber commented May 22, 2023

Without this we can't tell from a telemetry line whether a wallet hash identifies a destination wallet or the Station wallet.

Related: filecoin-station/desktop#714

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

As discussed in filecoin-station/zinnia#210, I prefer to have fine-grained configuration options rather than a named group like "production" or "running_in_station_desktop".

Since you need to detect what kind of wallet address is passed to Station Core, I propose introducing an env var specific to this particular aspect.

  • Env var name: WALLET_TYPE
  • Possible values: STATION-DESKTOP, DESTINATION and possibly more in the future.

I have a gut feeling that having a wallet type in our telemetry database will make it easier to build queries, compared to having a flag like "running_in_station_desktop" that needs to be applied by each query.

OTOH, I can see how it can be useful to see how was each Station Core instance deployed. Perhaps we can introduce another option for that? Then we use this new option to also detect Docker-based deployments.

  • Env var name: DEPLOYMENT (can we find a better one?)
  • Possible values: station-desktop, docker, standalone

WDYT?

@bajtos
Copy link
Member

bajtos commented May 22, 2023

Another idea:

Without this we can't tell from a telemetry line whether a wallet hash identifies a destination wallet or the Station wallet.

Which code/component is submitting the destination wallet into our telemetry data?

If we want to treat the destination wallet and the Station built-in wallet differently, I think using different field names for these two values is much better. E.g.

  • destination_wallet for the destination wallet configured in Station Desktop
  • wallet for the wallet provided to Station Core, either by the user running Core directly, or by Station Desktop (in which case it will be the built-in wallet).

@juliangruber
Copy link
Member Author

If we want to treat the destination wallet and the Station built-in wallet differently, I think using different field names for these two values is much better. E.g.

Station Core doesn't know whether the passed FIL_WALLET_ADDRESS refers to a destination wallet or a Station Desktop wallet. I think we have to introduce some variable for it to be able to tell.

Env var name: WALLET_TYPE

SGTM, this fixes the issue without introducing environments.

Env var name: DEPLOYMENT

Isn't that the same thing as I suggested? RUNNING_IN_STATION_DESKTOP=1 is DEPLOYMENT=station-desktop. If we made business logic changes based on the DEPLOYMENT key, I think that is using environments for configuration again, no?

@bajtos
Copy link
Member

bajtos commented May 22, 2023

If we want to treat the destination wallet and the Station built-in wallet differently, I think using different field names for these two values is much better. E.g.

Station Core doesn't know whether the passed FIL_WALLET_ADDRESS refers to a destination wallet or a Station Desktop wallet. I think we have to introduce some variable for it to be able to tell.

Oh, I think I didn't realise how exactly we are treating this address.

So IIUC:

  • When running Core in a standalone mode, the user provides the address of the wallet they are managing.
  • When running Core inside Station Desktop, Desktop provides the address of the built-in address.

This way, when a Station Module rewards the user for completing jobs, it sends the funds to the correct wallet.

However, if a user has a single wallet and runs both Station Desktop & Core (perhaps on two different computers):

  • The standalone Core will report the address of user's wallet (which is probably configured as the destination wallet in Desktop)
  • The Core running inside the Desktop will report the address of the built-in desktop wallet.

This whole situation seems confusing to me and I am not sure if adding a flag to distinguish between desktop-built-in and user-provided addresses increases the clarity enough. Maybe we should rethink why we collect the wallet address in our telemetry in the first place.

Having said that, I think using labels like "desktop-built-in" and "user-provider" will make our data easier to understand, compared to just adding a flag describing whether the data point with wallet address was reported from Core in Station Desktop or Core Standalone.

Env var name: WALLET_TYPE

SGTM, this fixes the issue without introducing environments.

👍🏻

Env var name: DEPLOYMENT

Isn't that the same thing as I suggested? RUNNING_IN_STATION_DESKTOP=1 is DEPLOYMENT=station-desktop. If we made business logic changes based on the DEPLOYMENT key, I think that is using environments for configuration again, no?

I don't want to change our business logic depending on this field, just report the value in telemetry and use it for our usage analytics. So that we know how many people run Core via Desktop/Docker/Standalone.

As I was thinking about this more, DEPLOYMENT is not a very good name. Few alternatives I like more: DEPLOYMENT_TYPE, DEPLOYMENT_MODE, OPERATION_MODE or even OPERATOR.

@juliangruber
Copy link
Member Author

  • The standalone Core will report the address of user's wallet (which is probably configured as the destination wallet in Desktop)
  • The Core running inside the Desktop will report the address of the built-in desktop wallet.

Almost: Desktop will also report the (hash of the) address of the destination wallet.

This whole situation seems confusing to me and I am not sure if adding a flag to distinguish between desktop-built-in and user-provided addresses increases the clarity enough. Maybe we should rethink why we collect the wallet address in our telemetry in the first place.

We are collecting wallet addresses in order to get a sense how how many users / identities use Station. A Station wallet doesn't count as an identity, because a user can run 5 Stations with the same destination address but will have 5 different Station addresses. Of course a user can also run 5 Station with 5 different destination addresses, but there our hands are tied.

Or let's ask the other way around: What do we lose by not collecting (hashes of) wallet addresses? We will only know the amount of Station instances, but have no sense of how many users / identities we have. It could be one person running 1000 nodes, we wouldn't know.

Having said that, I think using labels like "desktop-built-in" and "user-provider" will make our data easier to understand, compared to just adding a flag describing whether the data point with wallet address was reported from Core in Station Desktop or Core Standalone.

Agreed!

I don't want to change our business logic depending on this field, just report the value in telemetry and use it for our usage analytics. So that we know how many people run Core via Desktop/Docker/Standalone.

Gotcha!

@juliangruber juliangruber changed the title telemetry: add tag running_in_station_desktop telemetry: add tag deployment_type May 23, 2023
@juliangruber juliangruber requested a review from bajtos May 23, 2023 08:21
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

👍🏻

Should we also add the wallet type - user-provided vs. station-desktop-builtin? Let's do that in a new PR though, so that you can land & release this PR sooner.

@juliangruber
Copy link
Member Author

Should we also add the wallet type - user-provided vs. station-desktop-builtin? Let's do that in a new PR though, so that you can land & release this PR sooner.

Isn't that redundant?

  • DEPLOYMENT_TYPE = cli => WALLET_TYPE = user-provided
  • DEPLOYMENT_TYPE = docker => WALLET_TYPE = user-provided
  • DEPLOYMENT_TYPE = station-desktop => WALLET_TYPE = station-desktop-builtin

@juliangruber juliangruber merged commit 18ce10e into main May 24, 2023
@juliangruber juliangruber deleted the add/telemetry-running-in-station-desktop branch May 24, 2023 09:14
@juliangruber
Copy link
Member Author

@bajtos
Copy link
Member

bajtos commented May 25, 2023

Should we also add the wallet type - user-provided vs. station-desktop-builtin? Let's do that in a new PR though, so that you can land & release this PR sooner.

Isn't that redundant?

  • DEPLOYMENT_TYPE = cli => WALLET_TYPE = user-provided
  • DEPLOYMENT_TYPE = docker => WALLET_TYPE = user-provided
  • DEPLOYMENT_TYPE = station-desktop => WALLET_TYPE = station-desktop-builtin

It depends on who is looking at the data and how much they know about Desktop/Core architecture 🤷🏻

@juliangruber
Copy link
Member Author

You're right, but I don't expect anyone but the two of us to create Station Graphs atm

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.

2 participants