Skip to content

Diagnostics: Track endpoint hit#803

Merged
tonidero merged 18 commits into
diagnosticsfrom
diagnostics-track-endpoint-hit
Feb 22, 2023
Merged

Diagnostics: Track endpoint hit#803
tonidero merged 18 commits into
diagnosticsfrom
diagnostics-track-endpoint-hit

Conversation

@tonidero

@tonidero tonidero commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

Description

Based on #795
Deals with CSDK-655

Changes

  • Adds functionality to send an endpoint_hit event to the diagnostics endpoint on every request
  • Adds the Endpoint class with the enpoints we support, instead of hardcoding the path on each method.
  • Adds functionality to detect whether a response comes from the backend or the cache.

@tonidero tonidero added pr:feat A new feature WIP labels Feb 16, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed this test since the encoding part now is part of the EndpointTest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a name, which will be used when posting data to the diagnostics endpoint. I thought about posting the path template but that wasn't very readable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice 👍🏻
I did this refactor for iOS and it's been useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Was unused

@codecov

codecov Bot commented Feb 16, 2023

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (diagnostics@bcef86f). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 7d0ae23 differs from pull request most recent head 61e67af. Consider uploading reports for the commit 61e67af to get more accurate results

@@              Coverage Diff               @@
##             diagnostics     #803   +/-   ##
==============================================
  Coverage               ?   82.35%           
==============================================
  Files                  ?      130           
  Lines                  ?     4308           
  Branches               ?      547           
==============================================
  Hits                   ?     3548           
  Misses                 ?      543           
  Partials               ?      217           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tonidero tonidero marked this pull request as ready for review February 16, 2023 13:09
@tonidero tonidero requested a review from a team February 16, 2023 13:09
@tonidero tonidero removed the WIP label Feb 16, 2023
@tonidero tonidero changed the title Diagnostics track endpoint hit Diagnostics: Track endpoint hit Feb 16, 2023
Comment thread common/src/main/java/com/revenuecat/purchases/common/HTTPClient.kt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice 👍🏻
I did this refactor for iOS and it's been useful.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Hit" is a weird word for for this. Maybe trackRequest(endpoint:...)?

@tonidero tonidero Feb 17, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm what would you name the event that gets sent when we perform a request? request seems not descriptive enough. Maybe http_request_performed? This would be trackHttpRequestPerformedIfNeeded then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just did the rename. Lmk what you think!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto, trackRequest?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hit here on the other hand makes sense, for a cache "hit" vs "miss" 👍🏻

Comment thread common/src/main/java/com/revenuecat/purchases/common/networking/HTTPResult.kt Outdated
Base automatically changed from diagnostics-split-manager-tracker-synchronizer to diagnostics February 17, 2023 08:01
@tonidero tonidero force-pushed the diagnostics-track-endpoint-hit branch from 9c954f3 to dfa2575 Compare February 17, 2023 08:17
@tonidero tonidero requested a review from NachoSoto February 17, 2023 08:46

fun trackHttpRequestPerformed(
endpoint: Endpoint,
responseTime: Long,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does Kotlin have any sort of "Duration" type? It would be nice to encode in the type system that this is milliseconds.

@tonidero tonidero Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea! There is a Duration type indeed representing a difference in time. I think that could fit here well 👍

val payload: String,
val origin: Origin
) {
enum class Origin {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would be a good thing to add to iOS too.

@tonidero

Copy link
Copy Markdown
Contributor Author

Tests are failing due to SDKMAN but they are passing locally and since this is being merged to a non-main branch, will merge it for now. Will deal with those issues in that branch.

@tonidero tonidero merged commit e864f32 into diagnostics Feb 22, 2023
@tonidero tonidero deleted the diagnostics-track-endpoint-hit branch February 22, 2023 11:12
tonidero added a commit that referenced this pull request Feb 22, 2023
### Description
Based on #795 
Deals with [CSDK-655](https://revenuecats.atlassian.net/browse/CSDK-655)

### Changes
- Adds functionality to send an `endpoint_hit` event to the diagnostics
endpoint on every request
- Adds the `Endpoint` class with the enpoints we support, instead of
hardcoding the path on each method.
- Adds functionality to detect whether a response comes from the backend
or the cache.

[CSDK-655]:
https://revenuecats.atlassian.net/browse/CSDK-655?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
tonidero added a commit that referenced this pull request Feb 28, 2023
### Description
Based on #795 
Deals with [CSDK-655](https://revenuecats.atlassian.net/browse/CSDK-655)

### Changes
- Adds functionality to send an `endpoint_hit` event to the diagnostics
endpoint on every request
- Adds the `Endpoint` class with the enpoints we support, instead of
hardcoding the path on each method.
- Adds functionality to detect whether a response comes from the backend
or the cache.

[CSDK-655]:
https://revenuecats.atlassian.net/browse/CSDK-655?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
tonidero added a commit that referenced this pull request Feb 28, 2023
### Description
Based on #795 
Deals with [CSDK-655](https://revenuecats.atlassian.net/browse/CSDK-655)

### Changes
- Adds functionality to send an `endpoint_hit` event to the diagnostics
endpoint on every request
- Adds the `Endpoint` class with the enpoints we support, instead of
hardcoding the path on each method.
- Adds functionality to detect whether a response comes from the backend
or the cache.

[CSDK-655]:
https://revenuecats.atlassian.net/browse/CSDK-655?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
tonidero added a commit that referenced this pull request Mar 9, 2023
### Description
We broke posting receipts to the backend in #803 

This fixes it so we post to the correct URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:feat A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants