feat: adds metrics view events

[tdstein]

The metrics.views package coalesces the existing instrumentation APIs into a single interface. The new event type metrics.views.ViewEvent encapsulates the metrics.usage.UsageEvent and metrics.visits.VisitEvent. The ViewEvent schema is a mashup of the two underlying schemas, with the exception of the time field on VisitEvent. In ViewEvent this field is removed and mapped to started and ended. Otherwise, fields that do not exist in the underlying schema return None.

Resolves #137

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
927	891	96%	80%	🟢

New Files

File	Coverage	Status
src/posit/connect/metrics/init.py	100%	🟢
src/posit/connect/metrics/views.py	100%	🟢
TOTAL	100%	🟢

Modified Files

File	Coverage	Status
src/posit/connect/client.py	100%	🟢
src/posit/connect/resources.py	100%	🟢
TOTAL	100%	🟢

updated for commit: 3b7d501 by action🐍

[nealrichardson]

I'm struggling to read this, the tests aren't making clear to me how I (as a user) should use this and what I do with it.

[nealrichardson]

This doesn't seem right, right? I can't get here from Client?

[tdstein]

I removed it in favor of the Views interface, but I'm unsure if that's the right call. What is your opinion? Should we continue exposing Usage and Visits even though Views is a superset of them?

[nealrichardson]

I think we should wire them up all under metrics. Since they're in the API, it seems reasonable that we should provide a wrapper for them. Perhaps the user knows they only want one and not the other, and it's more efficient to just request the one they want. We prefer the one that just works and has everything, and we emphasize that in docs and guides.

As I was reading the API docs to try to understand the feature, I realized there's another place we probably want to expose this: on ContentItem, which would hit these APIs but filtered by guid. In that case, you won't want to query both endpoints because only one will work: one is for shiny (R only? IDK) and one is for everything else. That feels like a place where I don't want all three available, I don't want to have to think about which is right?

Does that make sense?

[tdstein]

It does. Based on my conversations with @kmasiello and other's it seems like understanding that aspect of the APIs is cumbersome. Aside from that, I really don't like the existing names. Maybe, namespace them under metrics.instrumentation (e.g., metrics.instrumentation.usage and metrics.instrumentation.visits).

Adding the hook in ContentItem is a great idea.

[nealrichardson]

Aside from that, I really don't like the existing names.

Yeah, agreed. Honestly I can't tell the difference between Views, Visits, and Usage.

Maybe, namespace them under metrics.instrumentation (e.g., metrics.instrumentation.usage and metrics.instrumentation.visits).

I guess you could 🤷 . So metrics.instrumentation would yield another object that has usage and visits attributes?

Do we need Views under metrics? Do we have other metrics? I guess this is top-level, so we could have other things like system metrics?

Would it make sense to put Views off of Content, even though that's not where they're mounted in the API? Like client.content.views.find()? They are views of content, yeah?

[tdstein]

Yeah, agreed. Honestly I can't tell the difference between Views, Visits, and Usage.

We should use this as an opportunity to improve the API. If the Views abstraction is well received, we should introduce that in a future version of Connect and drop the rest of the instrumentation API.

Do we have other metrics?

Nothing else at the moment, but we should. @christierney / @Lytol - what else are we pushing into Prometheus? It seems right to me to separate the namespace for metrics to declutter the non-metrics part of the API.

Would it make sense to put Views off of Content.

We can. It's the existing implementation plus a filter on content_guid.

[nealrichardson]

Yeah, agreed. Honestly I can't tell the difference between Views, Visits, and Usage.

We should use this as an opportunity to improve the API. If the Views abstraction is well received, we should introduce that in a future version of Connect and drop the rest of the instrumentation API.

Agreed. I just meant more about naming, I couldn't tell which was the one that contained the others since they all sounds the same. But if we're only presenting one name, I think it's fine.

Would it make sense to put Views off of Content.

We can. It's the existing implementation plus a filter on content_guid.

Let me clarify. I think there are two different ideas:

1. Add content.get(guid).views.find(), which is equivalent to the current metrics.views.find(content_guid = guid). That's from a previous comment.
2. What I meant here was: instead of client.metrics.views, should it be exposed via client.content.views and skip the metrics path entirely? If views are always about Content, maybe it makes logical sense to access them via content. (Or if we have other metrics to expose, we could do both?)

[tdstein]

What I meant here was: instead of client.metrics.views, should it be exposed via client.content.views and skip the metrics path entirely? If views are always about Content, maybe it makes logical sense to access them via content. (Or if we have other metrics to expose, we could do both?)

There are reasons to want to analyze the total usage across all applications. We don't have any other way of doing that other than collecting all views for every piece of content.

[tdstein]

Add #181

[nealrichardson]

Is this PR intended to solve #137?

[tdstein]

Is this PR intended to solve #137?

Yes

[tdstein]

I'm struggling to read this, the tests aren't making clear to me how I (as a user) should use this and what I do with it.

The main interface is client.metrics.views which has fine .find and .find_one. The returned object is the ViewEvent which contains all of the information from the instrumentation APIs.