refactor(backend): telemetry service interface

[BlairCurrey]

Changes proposed in this pull request

• abstracts counter/histogram metric recording behind new incrementCounter, incrementCounterWithAmount, recordHistogram.
  • in addition to doing the counter .add method, incrementCounterWithAmount handles the amount conversion and privacy obfuscation
• removes/makes private methods no longer used due to above refactor, leaving telemetry service interface with only: shutdown, incrementCounter, incrementCounterWithAmount, and recordHistogram methods

Context

Fixes: #2799

Checklist

• Related issues linked using fixes #number
• Tests added/updated
• Documentation added
• Make sure that all checks pass
• Bruno collection updated

[netlify]

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	a9feb43
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6697e27e1a07810008b6b6f9

[JoblersTune]

I'm also thinking about the wording incrementCounterWithAmount, we're always incrementing a counter by an amount. Maybe we could make it clearer by saying something like incrementCounterWithValue or incrementTransactionValueCounter or something else.

[JoblersTune]

Other than that LGTM, I'll just need to make a few tweaks with the introduction of my PR here as well. But now that I've seen all the changes that shouldn't be too big of a job :)

So let's get this in first. Then we have a clear plan on how to manage the conflicts.

[BlairCurrey]

I'm also thinking about the wording incrementCounterWithAmount, we're always incrementing a counter by an amount. Maybe we could make it clearer by saying something like incrementCounterWithValue or incrementTransactionValueCounter or something else.

I somewhat agree but I'm struggling to come up with something better because "amount" is what we call objects like this across our code, which is what Amount in incrementCounterWithAmount is referring to:

interface Amount {
  value: string
  assetCode: string
  assetScale: number
}

So I think part of the problem is we just use a generic name for our specific concept of the above Amount. But here we are.

This is why I don't really like incrementCounterWithValue - because we need to pass in something like Amount (with the asset info), not just the string/number/bigint value. In fact perhaps value would be a better name for the argument in incrementCounter to help distinguish it. In fact, I'm already using value in recordHistogram. I wouldn't want to specify that it's transaction specific either (such as incrementTransactionValueCounter), because this method should work with any sort of Amount (fees, limits, etc).

But still I don't love it as-is. incrementCounter takes an amount: number but incrementCounterWithAmount takes this different kind of amount (yuck).

And in this case the amount in incrementCounterWithAmount is not actually Amount, but { sourceAmount: bigint, sourceAsset: Asset }. This was driven by the underlying convert call but I think not well-named (amount.sourceAmount kinda sucks in this context because it's the value not an Amount object and there is no non-source amount).

I'm warming up to something more like this which changes the type of amount in incrementCounterWithAmount and renames amount to value in incrementCounter. I think these keeps things consistent with our naming conventions and helps distinguish between the two increment counter methods. What do you think?

export interface TelemetryService {
  incrementCounter(
    name: string,
    value: number,
    attributes?: Record<string, unknown>
  ): void
  incrementCounterWithAmount(
    name: string,
    // value could be string or number instead, depending on what ends up making the most sense
    amount: { value: bigint, assetCode: string, assetScale, number },
    attributes?: Record<string, unknown>
  ): Promise<void>
 }

Or does it still seem unclear because we're always incrementing an amount in the generic sense? I could see that, but I would argue we need a better name for our amount like AssetAmount or something. But this introduces a new name for the same thing we have all over the place which I don't like. 🤷

[mkurapov]

we can delete the setting of this.meterProvider on L74 now

[BlairCurrey]

whoops, yes. good catch

[mkurapov]

we can also get rid of the getBaseAssetCode & getBaseAssetCode here as well?

[BlairCurrey]

yup, removed

[mkurapov]

I think incrementCounterWithAmount is ok based on the signature, maybe a (long) alternative is incrementCounterWithTransactionAmount?

Likewise, we can also have the incrementCounter function not take in a value: number, since incrementing counter implies its going up by one? Then we can have a private method just use '1' under the hood when doing the actual metric incrementation.

Not too big of a deal though.

[JoblersTune]

I was also leaning toward incrementCounterWithTransactionAmount then. It's better to have something long but clear in my opinion.

[BlairCurrey]

@JoblersTune @mkurapov updated to incrementCounterWithTransactionAmount.

We can leave it there if we like, although I still wonder if this is making the function name more opinionated (that the amount is a transaction amount) than the actual function body does (and should). I mean shouldn't this same method be used for other amounts, such as fees (which might actually be right around the corner)? In which case the call would be something like:

incrementCounterWithTransactionAmount('fees_amount', feeAmount)

I think the function should signify this is our concept of amount and avoid signify its for a specific use-case. The incrementCounterWithAmount name doesnt signify this is our amount currently, but the whole function signature (with arg type) does. Again, I wish we had a less generic term than "amount" for the value/asset pair concept. AssetAmount maybe, which would make this incrementCounterWithAssetAmount. I guess I dont expect that to make sense to anyone either though, since its a new convention and elsewhere we just use "amount".

Likewise, we can also have the incrementCounter function not take in a value: number, since incrementing counter implies its going up by one? Then we can have a private method just use '1' under the hood when doing the actual metric incrementation.

Didn't do this one... our incrementCounter is calling counter.add under the hood so I think we should allow passing in whatever we need to. We are using this currently in incrementCounterWithTransactionAmount and could conceivable use it outside the service via incrementCounter as well. Also seems incongruent to assume 1 in incrementCounter but then not incrementCounterWithTransactionAmount IMO. I agree there is some un-intuitiveness but for me this traces back to the opentelemetry api. I don't think "incrementing" (our word choice) implies +1, but I do think a "counter" (their name) kind of does - and obviously there is nothing we can do about that. Perhaps if anything we change our name from incrementCounter -> addToCounter (and similar).

[JoblersTune]

@BlairCurrey wouldn't we use a different exposed function for fees? Since I assume we don't need to do privacy stuff on fees, only asset conversions. In which case incrementCounterWithTransactionAmount and incrementCounterWitFeesAmount would arguably add more clarity?

[mkurapov]

Didn't do this one... our incrementCounter is calling counter.add under the hood so I think we should allow passing in whatever we need to.

Makes sense, I think that's fine.

@BlairCurrey @JoblersTune for privacy preserving amounts, we can have
incrementCounterWithPrivacyTransactionAmount or just add a preservePrivacy arg into incrementCounterWithTransactionAmount

[BlairCurrey]

@BlairCurrey wouldn't we use a different exposed function for fees? Since I assume we don't need to do privacy stuff on fees, only asset conversions. In which case incrementCounterWithTransactionAmount and incrementCounterWitFeesAmount would arguably add more clarity?

That's a fair point... I feel like an new arg (like @mkurapov suggested) might work well since everything else in the function would be the same (doesnt really add any complicated logic and will re-use the conversion stuff). But I could also see a new explicit method if the difference ends up being more than that... I guess we can roll with this for now and cross that bridge when we come to it.

[mkurapov]

Looks good, just a single comment

[mkurapov]

I think we should actually move these promises into a Promise.all

[BlairCurrey]

they arent promises 😄. the underlying add/record sdk methods are synchronous.

incrementCounterWithTransactionAmount returns a promise due to the amount conversion.

[mkurapov]

Ah, that's right. LGTM now