Skip to content

doubly escape periods in counter names so we dont create invalid json#3000

Closed
bbeaudreault wants to merge 1 commit intovitessio:masterfrom
HubSpot:counters_valid_json
Closed

doubly escape periods in counter names so we dont create invalid json#3000
bbeaudreault wants to merge 1 commit intovitessio:masterfrom
HubSpot:counters_valid_json

Conversation

@bbeaudreault
Copy link
Contributor

@bbeaudreault bbeaudreault commented Jul 18, 2017

This fixes an issue created by #2992. This broke our automation and monitoring which relies on the json response of /debug/vars being proper json.

cc @nerdatmath @sougou

@bbeaudreault
Copy link
Contributor Author

By the way, I found the original escaping change to be unnecessary, though I can work around it once the json is valid. Since we know the format of the keys, we could still properly split the keys accounting for potential periods. It was a bit involved, but worked:

# for maps where one of the keys is the username, the username may have periods in it. 
# Account for that in how we parse split on periods below.
idx = tag_list.index('user')
if idx == 0:
    # no username, just split from right
    tag_data = name.rsplit(key_split_char, len(tag_list) - 1)
elif idx == len(tag_list) - 1:
    # username is the last item in the list, split from left
    tag_data = name.split(key_split_char, len(tag_list) - 1)
else:
    # limited split from left, up to where the username is
    lparts = name.split(key_split_char, idx)
    # limited split of remainder from right, backwards to username
    rparts = lparts[idx].rsplit(key_split_char, len(tag_list) - idx - idx)
    # contains everything up to but not including username
    tag_data = lparts[:idx]
    # add rparts on, the first item will be the username with dots in it
    tag_data += rparts

@michael-berlin
Copy link
Contributor

@bbeaudreault Congrats on pull request #3000 ! :) Sugu got #1000 and I #2000 back then :D

@bbeaudreault
Copy link
Contributor Author

Woohoo!

@bbeaudreault
Copy link
Contributor Author

bbeaudreault commented Jul 19, 2017

Another bonus is that this year we hit 1000 PRs a month early!

@bbeaudreault
Copy link
Contributor Author

bbeaudreault commented Jul 20, 2017

@sougou how long should we give @nerdatmath to respond here before we just merge this? As far as I can tell the original PR broke the /debug/vars for all existing users who try to parse it as json, in at least python and go. So this may break any specific parsing he's added, but will fix everyone else.

@sougou
Copy link
Contributor

sougou commented Jul 20, 2017

I want to see if the previous change can be rolled back in favor of an internal fix. I'll try to catch @nerdatmath when he's online to chat about it.

@bbeaudreault
Copy link
Contributor Author

Sounds good, thanks

@nerdatmath
Copy link
Contributor

nerdatmath commented Jul 23, 2017

The problem's not in my change, although my change revealed it. The problem is in the String method of Counters. Compare it to the String method of expvar.Map, and you'll see that where they use %q (which does the escaping required to generate valid JSON), we use "%v", which is equivalent to "%s" in this case because keys are always strings, and %s doesn't do any escaping.

#3005 sent to fix Counters.String.

@bbeaudreault
Copy link
Contributor Author

This was fixed more correctly by #3005

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.

5 participants