Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send packet only with POST method. #470

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nostromoo
Copy link

@nostromoo nostromoo commented Jan 23, 2024

Closes #468

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a6a7260) 87.12% compared to head (72de688) 86.99%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #470      +/-   ##
============================================
- Coverage     87.12%   86.99%   -0.14%     
+ Complexity      364      358       -6     
============================================
  Files            32       32              
  Lines          1429     1422       -7     
  Branches        165      162       -3     
============================================
- Hits           1245     1237       -8     
- Misses          115      116       +1     
  Partials         69       69              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

int packets = (int) Math.ceil(events.size() * 1.0 / PAGE_SIZE);
List<Packet> freshPackets = new ArrayList<>(packets);
for (int i = 0; i < events.size(); i += PAGE_SIZE) {
List<Event> batch = events.subList(i, Math.min(i + PAGE_SIZE, events.size()));
final Packet packet;
if (batch.size() == 1) packet = buildPacketForGet(batch.get(0));
Copy link
Collaborator

@hannesa2 hannesa2 Jan 24, 2024

Choose a reason for hiding this comment

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

@d4rken
When I understand right, you did in 1f8ccd6 this decision if it's a get or a post by asking batch.size(), am I right ?

Do you agree with these changes ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember why I did it this way 🤔.

Might have been an optimization, i.e. GET having less overhead than POST for single events. In hindsight I don't think the overhead would be huge enough to warrant both GET and POST.

I'm neutral on this change.

@nostromoo Please consider adding some descriptions to your PRs. What made you change this?

@hannesa2 hannesa2 requested a review from d4rken January 24, 2024 05:54
Copy link
Collaborator

@hannesa2 hannesa2 left a comment

Choose a reason for hiding this comment

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

Like @d4rken I'm neutral on this change too.
We can give it a try

@hannesa2
Copy link
Collaborator

@nostromoo
Please test it first
implementation 'com.github.nostromoo:matomo-sdk-android:remove_get_packet-SNAPSHOT'
if it comes now with desired behavior. If yes, I'll merge it.

Please keep me informed

@d4rken
Copy link
Member

d4rken commented Jan 24, 2024

Now I see the linked #468.

Did something change on the server-side? According to docs (the last time I checked) the server should accept both GET and POST. 🤷

This could warrant further debugging to find out why GET is not working. Broken in the SDK? Or an issue with the server config?

If it's the later, changing the SDK seems like the wrong way to fix it 😁.

@hannesa2
Copy link
Collaborator

Now I see the linked #468.

I did it

@hannesa2
Copy link
Collaborator

hannesa2 commented Feb 7, 2024

If it's the later, changing the SDK seems like the wrong way to fix it 😁.

Any suggestion what's the right way

@d4rken
Copy link
Member

d4rken commented Feb 7, 2024

If it's the later, changing the SDK seems like the wrong way to fix it 😁.

Any suggestion what's the right way

Check if single GET requests are reaching the server, and if they are, check why the server is not processing them. Removing the GET and only using POST is fine, but shouldn't be done as a "guess" without understanding why GET is not working.

@mattab Did the server's behavior for GET change?

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.

Single event doesn't appear on Dashboard
4 participants