Skip to content

Conversation

prayansh
Copy link
Contributor

@prayansh prayansh commented Mar 1, 2022

  • Add a new destination metadata plugin that captures _metadata info for Segment to handle delivery of events to cloud/device destinations
    • We keep the old format of integrations object being used by customers to control delivery
  • Switch delivery of events from /batch to /b endpoint
  • Add a writeKey field to the batch event file

@prayansh prayansh requested a review from wenxi-zeng March 1, 2022 02:03
// the userId tied to the event
abstract var userId: String

abstract var _metadata: DestinationMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

an unrelated but more for maintainability question: should we only use abstract for EventType and mark all the other fields as lateinit? because all the other fields have the same implementation in the subclasses. if they're not abstract, we will have less redundant code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh i didnt realize fields in an abstract class can be set as lateinit. That sounds like a good idea maybe. But I think that change goes beyond the scope of this PR. I will address those in another one after tho

fun upload(apiHost: String): Connection {
val connection: HttpURLConnection = openConnection("https://$apiHost/batch")
val connection: HttpURLConnection = openConnection("https://$apiHost/b")
connection.setRequestProperty("Content-Type", "text/plain")
Copy link
Contributor

Choose a reason for hiding this comment

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

seems to be redundant, since createPostConnection internally set this to gzip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content-Encoding is being set to gzip, this is Content-Type which I believe has a different meaning

Copy link
Contributor

Choose a reason for hiding this comment

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

ah. I read too fast. thought they are both Content-Type


internal fun HttpURLConnection.createPostConnection(): Connection {
val outputStream: OutputStream
setRequestProperty("Content-Encoding", "gzip")
Copy link
Contributor

Choose a reason for hiding this comment

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

any benefits to move this line from upload to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just felt logically connected to the next line, that actually does the gzip encoding, so I moved it here

@prayansh prayansh force-pushed the pray/dest-metadata branch from 86a0c61 to 1118ce5 Compare March 1, 2022 18:43
@codecov-commenter
Copy link

Codecov Report

Merging #73 (5edec97) into main (8ab7fc1) will increase coverage by 0.31%.
The diff coverage is 81.45%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #73      +/-   ##
============================================
+ Coverage     57.56%   57.88%   +0.31%     
- Complexity      416      426      +10     
============================================
  Files            67       69       +2     
  Lines          7204     7296      +92     
  Branches        690      708      +18     
============================================
+ Hits           4147     4223      +76     
  Misses         2549     2549              
- Partials        508      524      +16     
Impacted Files Coverage Δ
...analytics/kotlin/core/utilities/Base64UtilsTest.kt 100.00% <ø> (ø)
...n/java/com/segment/analytics/kotlin/core/Events.kt 46.23% <33.33%> (-1.14%) ⬇️
...core/platform/plugins/DestinationMetadataPlugin.kt 66.66% <66.66%> (ø)
...m/segment/analytics/kotlin/core/HTTPClientTests.kt 88.88% <80.00%> (+0.25%) ⬆️
...platform/plugins/DestinationMetadataPluginTests.kt 91.80% <91.80%> (ø)
...egment/analytics/kotlin/android/EventsFileTests.kt 100.00% <100.00%> (ø)
...m/segment/analytics/kotlin/android/StorageTests.kt 85.29% <100.00%> (+0.14%) ⬆️
...va/com/segment/analytics/kotlin/core/HTTPClient.kt 82.60% <100.00%> (-1.22%) ⬇️
...kotlin/core/platform/plugins/SegmentDestination.kt 55.26% <100.00%> (-0.74%) ⬇️
...alytics/kotlin/core/utilities/EventsFileManager.kt 93.24% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ab7fc1...5edec97. Read the comment docs.

@prayansh prayansh merged commit ed398f0 into main Mar 3, 2022
@prayansh prayansh deleted the pray/dest-metadata branch March 3, 2022 00:30
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.

3 participants