[FEATURE] Added meta section to each request for tracking and telemet…

[karoltarasiuk]

…ry purposes

LRN-20220

[shtrom]

Looks like a good start. It would be good to add a test for completeness. I'm also unsure why the testsuite fails with Travis. Does it work locally for you?

[karoltarasiuk]

@shtrom I run them with make test and yes - they fail because of those changes. I didn't actually focus on it at all, as it was a wip, but definitely will add some coverage and then fix all the failures.

[karoltarasiuk]

@shtrom fyi tests fixed

[shtrom]

Yes, I think we got it! There's a few bits of cleanup and commenting needed, but I think it's good to go otherwise. Now we need to test it against the backend (:

If you're game, I noticed the test providers look very similar. I wonder if we could distill them down to something like initWithTelemetryProvider and initWithoutTelemetryProvider which would pass the Init object around, and it would be up to the test-case to actually call the method they want to test on it.

I haven't thought that entirely through, so it may not actually be practical, and you may also have better things to do, so that bit's up to you (:

[karoltarasiuk]

Cheers @shtrom, was a lot back and forth, thanks for the patience. Definitely may have some other things to do but will wait til tomorrow as may have some downtime after all.

One thing Peter mentioned to me recently is also what I questioned during our call, to try to avoid adding meta/telemetry to signed request.

I know that in Author API it wouldn't be that hard as we have options with request and signature passed separately: https://github.com/Learnosity/api-author/blob/81f9e8c211c22796d4db8390adbdc627b10bb185/www/latest/app/app.js#L30-L35 We could easily add one more property like meta there, instead of putting it within the request. But would need input from other teams because most likely some changes to other APIs would be needed :/

So the question is - do we keep meta in the request, or spend more time and change some APIs as meta don't really belong in signed request?

[shtrom]

It's an excellent question.

I think the problem stems from the fact the initOptions that we pass to the JS application is only the request. Without changing the APIs to support an additional telemetry argument (and keeping in mind that some also have events), I'm not sure this would work, and I'd be worried about backward compatibility with the SDK generating telemetry arguments that older APIs don't support in their Init.

I'm not fully across the JS init for all the APIs, so I may be missing something that would otherwise be obvious.

[karoltarasiuk]

Yeah, so in case of Author API initOptions object is actually generated and passed in this format:

initOptions: {
    request: { ... },
    security: { ... }
}

Items Reports and Events APIs are from what I see the same. Data API also treats request and security separately.

In this case it would be easy to just another key to this object: meta.

Only Questions API and Assess API differ a bit, and we would need to look closely, but may not be too bad...

[shtrom]

Ah, yeah definitely. This makes sense.

Thinking a bit more about it, though, I don't know how chainloading of APIs would work: if Items initialises Assess, we probably want the meta somewhere that Assess will be able to get to, and have that come with a valid signature.

[sebablanco]

I don't know we need to worry about passing through this metric to dependant APIs. The client only signed the parent API, so that should be enough.

[shtrom]

Not strictly necessary, yeah. However the dependent APIs would still be initialised, but without telemetry information, which we would capture as a pre-telemetry SDK.

This may create noise in the data. Unless we have another way to tell that an API has been initialised as dependent, and ignore that?

[karoltarasiuk]

@shtrom just to answer your last question - yes we do. In Question Editor we initialise Questions API and this is how we pass the metricsContext information to it, so it knows where are we coming from: https://github.com/Learnosity/api-questioneditor/blob/75f84ee41b62ed78f124a56027e47d344cb40e68/www/latest/app/models/questionsApi.js#L88 (btw there is another context set, but it's quite legacy so don't worry about it)

metricsContext is an array and is being populated here: https://github.com/Learnosity/api-questioneditor/blob/75f84ee41b62ed78f124a56027e47d344cb40e68/www/latest/app/app.js#L59

@stellalie has all the info how is it tracked.

[karoltarasiuk]

Based on my Slack conversation with @shtrom I will pass this ticket to functional in its current shape, and we will do regression testing for all APIs. After that it will wait not merged until other moving parts are done.