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

Pass visited URL as query parameter #40

Closed
wants to merge 1 commit into from

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Oct 24, 2021

A POC for my suggestion at #37 (comment)

I have almost no knowledge of Go and couldn't get the Makefile working (I get an Makefile:5: .config/makefile.env: No such file or directory error). So please let me know if I need to fix more things :)

@ihucos
Copy link
Owner

ihucos commented Oct 25, 2021

Hello wouterj,

thank you very much for the pr! The .config/makefile.env file you need to create with some dummy values as a quickfix


CLOUDFLARE_ZONE1=dummy
CLOUDFLARE_ZONE2=dummy
CLOUDFLARE_TOKEN=dummy

I need to make this optionally.

But in any case regarding the pr content:

One of the ideas behind counter is that the client is responsible for defining what an unique view is and basically only send an http request once. There are multiple layers or fences if you want to ensure that. At first we save something in the sessionStorage, in case that fails we rely on HTTP caching for the request to not actually arrive to the server. Now with that change we would fix the apparently unreliable Referer header but we would also have a different url for each different page being requested to the counter server in the case where localSession fails. This different URLs would "bust" trough the HTTP cache that was set up as an additional "defense line".

I am not sure but at the moment I see more reliably counting unique visitors more important than reliably counting referers. It might be worth to first look into why exactly the Referrer header is not "working", If I remember correctly this can be dependent from another header. In any case I think I would rather document and somehow manage the problem instead of fixing it in a way that shortens the reliability of unique views.

What you think, sounds reasonable? Feel free to disagree or complement. Maybe some data or something with more evidence would also make the picture at least for me clearer.

Cheers and thanks

@wouterj
Copy link
Contributor Author

wouterj commented Oct 25, 2021

Thanks for the fast and detailed response!

I need to make this optionally.

Thanks. An idea to fix this: I mostly work on PHP open source projects, in there it's common to create a file suffixed with .dist (makefile.env.dist) with these "dummy values" and then add the real file (makefile.env) to the .gitignore. This way, everyone can cp the dist file to the real file and have a working system (and it allows you to modify the entered values, so you can use the real values on real deployments).

At first we save something in the sessionStorage, in case that fails we rely on HTTP caching for the request to not actually arrive to the server.

Oh, that's smart! I see why PR wouldn't work in this case.

I am not sure but at the moment I see more reliably counting unique visitors more important than reliably counting referers. It might be worth to first look into why exactly the Referrer header is not "working"

I think there is a minor misunderstanding here: there is a difference between the referrer form value (line 86) and the Referer header (line 92). The first one is used to pass the referrer for this visit and I agree that this is indeed "nice to have" but not important.
The second one (the one modified in this PR) is used to determine the entry page of this visitor.

My use-case for counter is on my personal blog. I'm most interested in which blog posts receive visits (so I can see which topics are popular and which posts I must somewhat keep up to date). Always seeing the / entry page (even when a visitor is visiting a specific blogpost as entry - which happens with most of my visitors) is a real deal breaker for me.


You can see the issue with the Referer header (used for the entry page) when visiting a specific blog post of my website with the network tab open: https://wouterj.nl/2021/09/symfony-6-native-typing You'll see the Referer header of /track is always https://wouterj.nl/.

Now I see why query parameters aren't used, maybe we can introduce a custom request header? E.g.

fetch("https://counter.dev/track?"+new URLSearchParams({referrer:document.referrer,screen:screen.width+"x"+screen.height,user:"...", {
  headers:{"X-Page":window.location.href}
)

@ihucos
Copy link
Owner

ihucos commented Oct 29, 2021

Nice idea with using a custom HTTP request header, give me a couple of days to test it and research a little - don't want to disturb production users.

@ihucos
Copy link
Owner

ihucos commented Oct 29, 2021

I added in the README how to run the project and removed the requirement of needing the ./config/makefile.env file - but you already worked that out

@ihucos ihucos marked this pull request as ready for review October 29, 2021 08:18
@ihucos
Copy link
Owner

ihucos commented Nov 4, 2021

Deployed optional support on the backend: c9e496e

Testing it on the site itself: 670d732#diff-8abeb5fbc05848651c162faa817ac5d8edccf139c1d86fb9986c98092fddf093R76

Will change tracking code once expected functionality looks plausible.

@wouterj
Copy link
Contributor Author

wouterj commented Nov 13, 2021

Thank you from taking over this idea @ihucos!

I've also implemented this header on my blog. For some reason, the entry pages are still / (although I can see from the referrer that they really did not enter the website on the homepage). I'll continue digging again.

@ihucos
Copy link
Owner

ihucos commented Nov 13, 2021

Hello wouterj,

hmm, I just created a dummy account, here is it's shared dashbaord: https://counter.dev/dashboard.html?user=testtest2&token=VNXu9mahYW%2BCm%2FcU

When you issue the following command:

curl 'https://counter.dev/track?site=testtest2' -H "origin: example.com" -H "X-Referer: /test" 

The specified X-Referer is showed uder "Entry Pages".

Speculation:
Maybe the problem is on the JavaScript side and window.location.href does not always work depending on something that I have no idea right lnow? Think I changed your code a bit and it was called X-Page before, not sure which is better btw but now its X-Referer

@wouterj
Copy link
Contributor Author

wouterj commented Nov 14, 2021

Thank you, I made a dumb mistake (added the header object to the URLSearchParams() argument list instead of the fetch()). I guess it works like expected now.

I'm closing this PR, as your changes in the master branch achieve what I tried to do in this PR in a better way.

@wouterj wouterj closed this Nov 14, 2021
@wouterj wouterj deleted the loc-param branch November 14, 2021 13:21
@wouterj
Copy link
Contributor Author

wouterj commented Nov 14, 2021

Hmm, no it isn't.

As my website uses a static generator, I'm now even hard-coding the X-Referer header value (see e.g. https://wouterj.nl/2021/11/deprecations-are-not-errors). And I removed the counter snippet from my homepage (only rendering it for blog posts).

Still, I'm getting only / entry pages: image

(fyi, I posted a tweet linking to this blogpost yesterday. So I'm 100% sure that all t.co referrers are actually pointing to this blogpost)

@ihucos
Copy link
Owner

ihucos commented Nov 16, 2021

That is weird. I sanity checked and your javascript is right. It should work, but I cannot reproduce in the written minimal example. Hmmm if you hard coded the value its not window.location.href that is off - that is good to know.

Weird, not sure how to debug it

@ihucos
Copy link
Owner

ihucos commented Nov 16, 2021

It's that :-)
Screenshot from 2021-11-16 22-18-37

@ihucos
Copy link
Owner

ihucos commented Nov 16, 2021

https://stackoverflow.com/questions/25727306/request-header-field-access-control-allow-headers-is-not-allowed-by-access-contr

For cross-domain requests, setting the content type to anything other than application/x-www-form-urlencoded, multipart/form-data, or text/plain will trigger the browser to send a preflight OPTIONS request to the server.

Can't aimplement handling OPTIONS on the server as this would substantially decrease the throughput of tracked visits the server can handle. @wouterj can you try adding the Content-Type: text/plain header?

@wouterj
Copy link
Contributor Author

wouterj commented Nov 27, 2021

Unfortunately, this still triggers the CORS error. So it seems like a custom header is not the solution here :/

@ihucos
Copy link
Owner

ihucos commented Nov 29, 2021

Gotcha, thanks for trying it out

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.

None yet

2 participants