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

Adds cdt parameter to tracking query items #301

Merged
merged 3 commits into from Sep 3, 2019
Merged

Adds cdt parameter to tracking query items #301

merged 3 commits into from Sep 3, 2019

Conversation

notjosh
Copy link
Contributor

@notjosh notjosh commented Jul 16, 2019

Because of ✨reasons✨, the h/m/s values don't seem to be used in calculating "time on page" in the backend, but cdt does.

This PR attaches cdt to the tracking events in addition to the existing h/m/s values.

According to this issue, cdt will be used to calculate the "time on page" correctly for the clients, regardless of when they're sent to the server

According to these docs, the format is yyyy-MM-dd HH:mm:ss (i.e. 2011-04-05 00:11:42) - not quite ISO-8601, but close (although we could use a unix timestamp? They're always a little harder to grok for human eyes though). However (and this is an implementation detail, so probably best to ignore or update docs), the server uses unix timestamp internally, and processes it via strtotime(), so we could use a formal ISO 8601 date string here if we'd prefer. ➡️ Using ISO 8601 format since 2feaae1


Resolves #299

@brototyp-bot
Copy link

brototyp-bot commented Jul 16, 2019

1 Warning
⚠️ Are there any changes that should be explained in the README.md?

Generated by 🚫 Danger

@brototyp
Copy link
Member

Hey @notjosh, this looks great! I am currently trying to get one of the backend developer to check if it's fine to send a "normal" ISO-8601 date along which I would like the most. If we don't hear back from them soon I am happy to merge it though. I'll get back to you within a day.

@notjosh
Copy link
Contributor Author

notjosh commented Jul 17, 2019

Great! No rush though, I can use my branch for as long as necessary :)

@tsteur
Copy link
Member

tsteur commented Jul 18, 2019

something to keep in mind... there is this setting:

; By default, Matomo accepts only tracking requests for up to 1 day in the past. For tracking requests with a custom date
; date is older than 1 day, Matomo requires an authenticated tracking requests. By setting this config to another value
; You can change how far back Matomo will track your requests without authentication. The configured value is in seconds.
tracking_requests_require_authentication_when_custom_timestamp_newer_than = 86400;

So if the cdt is older than one day and no token is specified, Matomo will throw an exception and not record the request. Don't know if there's a feature that eg sends tracking requests while the user was offline eg a day later... these would not be recorded then.

@tsteur
Copy link
Member

tsteur commented Jul 19, 2019

I haven't tested ISO8601 string, timestamp should work well though.

@brototyp
Copy link
Member

Thanks for your feedback @tsteur!

@notjosh, I guess I would change over to using a timestamp here. I would give "easier code" more weight than the "readability of the requests". What do you think?

@notjosh
Copy link
Contributor Author

notjosh commented Jul 19, 2019

I was looking through the Android code for other reasons earlier, but it applies here:

So if the cdt is older than one day and no token is specified, Matomo will throw an exception and not record the request. Don't know if there's a feature that eg sends tracking requests while the user was offline eg a day later... these would not be recorded then.

and

I haven't tested ISO8601 string, timestamp should work well though.

Android is already attaching cdt to all requests, and is using a date format closer to ISO 8601 (though, it's different than the format documented).

Either it's ok to do that here, or it should probably be dropped from Android too, so we're not seeing inconsistent results across platforms.

@notjosh, I guess I would change over to using a timestamp here. I would give "easier code" more weight than the "readability of the requests". What do you think?

Is it really much different? It's still (presumably) to be hidden behind a DateFormatter helper, so the caller won't see any difference, and the formatter itself (using .dateFormat is fairly common?

Ultimately, I don't mind either way - I call the SDK, and you decide how to send those dates :) But at the moment it's fairly inconsistent across platforms, so it might be helpful to align those internally across teams.

@brototyp
Copy link
Member

Hey @notjosh, I had a chat with the backend engineers. ISO8601 is fine. Let's change over there (if you are fine as well). I'll try to get the documentation updated in that regard.

@notjosh
Copy link
Contributor Author

notjosh commented Jul 31, 2019

Hiya, I've bumped to use ISO 8601 date formatter (in 2feaae1).

It's trying to use ISO8601DateFormatter when it's available, which might add some complexity to the code you don't want - I'm happy to remove that if you'd prefer.

@notjosh
Copy link
Contributor Author

notjosh commented Aug 30, 2019

Gentle bump. Any progress on this? Happy to maintain a fork (we need the functionality!) if it's a Bad Idea on your side though.

@brototyp
Copy link
Member

Hey @notjosh, thanks for your patience. And thanks for the bump. I did forget about your PR here.
I am happy with your changes. Can you do me a favor and rebase it from develop, check the changelog (I think your editor replaced all bulletpoint - by *s)? Once the tests go though I'll merge through. This could then be part of the Swift-5 release.

@notjosh
Copy link
Contributor Author

notjosh commented Sep 2, 2019

Oh, doh, you're right - Prettier bumped all the bullets. I rebased, fixed the changelog, and cleaned out the merge commit. Should be ok from my side, assuming tests are green

@brototyp brototyp merged commit b856ddc into matomo-org:develop Sep 3, 2019
@brototyp
Copy link
Member

brototyp commented Sep 3, 2019

Perfect. Thanks a lot!

@brototyp
Copy link
Member

brototyp commented Sep 3, 2019

This just got released in version 7.0.0

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.

Page view time often recorded as 0 seconds when batched
4 participants