Skip to content

Adds the possibility to vary the template of tablet debug urls.#3842

Merged
michael-berlin merged 4 commits intovitessio:masterfrom
mpawliszyn:mikepaw.format-debug-urls
Apr 25, 2018
Merged

Adds the possibility to vary the template of tablet debug urls.#3842
michael-berlin merged 4 commits intovitessio:masterfrom
mpawliszyn:mikepaw.format-debug-urls

Conversation

@mpawliszyn
Copy link
Copy Markdown
Collaborator

@mpawliszyn mpawliszyn commented Apr 17, 2018

Sometimes the way you get to a tablet from a webpage is different from how a machine does it from within the same part of the network fabric.

@mpawliszyn mpawliszyn force-pushed the mikepaw.format-debug-urls branch 3 times, most recently from e32b7ef to 5c32594 Compare April 17, 2018 21:16
Copy link
Copy Markdown
Contributor

@michael-berlin michael-berlin left a comment

Choose a reason for hiding this comment

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

Change looks good to me after my comment is addressed.

Do you actually need to customize the whole path? A prefix would not be good enough?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not good because you would crash the vtgate at runtime.

Instead, you could make the template a global variable and set it during init().

Also, please use Exitf instead of Fatalf: https://godoc.org/github.com/golang/glog#Exitf

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah thanks for the tip. It's been fixed I think. I'm clearly still green behind the ears.

@mpawliszyn mpawliszyn force-pushed the mikepaw.format-debug-urls branch from 5c32594 to 7fd2143 Compare April 18, 2018 13:57
@mpawliszyn
Copy link
Copy Markdown
Collaborator Author

@michael-berlin I think all that I'm customizing is the prefix, or that the prefix is all that is included in the link. If you have any recommended improvements with that context then I'd happily do them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We (at Slack) have a similar issue and have never put the time into addressing it so thanks for this feature.

However I'd like to be able to format an URL based on just the hostname without the port. From my brief understanding of the code it seems like I could do this with "http://{{.GetHostNameLevel 0}}.some.domain" -- if so that would be perfect, but if not then it'd be great to enable this as part of this change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

GetHostNameLevel is there to help divide up the hostname into it's components. If you want the whole thing I think http://{{.Tablet.Hostname}} should work.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I actually just tested this out and it indeed works.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like this is a non-trivial feature. How about leaving a pointer in this help text for interested users?

e.g. add: "See the Go code for getTabletDebugURL() how to customize this."

And then you can document the feature and tricks like {{.Tablet.Hostname}} there.

Copy link
Copy Markdown
Contributor

@michael-berlin michael-berlin left a comment

Choose a reason for hiding this comment

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

Having a full fledged template here seems to be preferred over adding a simple prefix. That works for me.

Can you please address my comments? After that I can merge it. Thank you :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is currently unused. I assume you want to use it in your custom templates?

Please document how to use this and why it must be kept in the code.

Also, please add a test for this method. This will better demonstrate the intended use and also make sure that this functionality won't be broken.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like this is a non-trivial feature. How about leaving a pointer in this help text for interested users?

e.g. add: "See the Go code for getTabletDebugURL() how to customize this."

And then you can document the feature and tricks like {{.Tablet.Hostname}} there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please move this to the first group of imports?

@mpawliszyn mpawliszyn force-pushed the mikepaw.format-debug-urls branch from 7fd2143 to 7206854 Compare April 19, 2018 18:17
@mpawliszyn
Copy link
Copy Markdown
Collaborator Author

@michael-berlin I think I've addressed all of your comments now.

@mpawliszyn mpawliszyn force-pushed the mikepaw.format-debug-urls branch from 7206854 to 02e7657 Compare April 19, 2018 18:21
mpawliszyn and others added 4 commits April 25, 2018 10:06
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>
Signed-off-by: Michael Berlin <mberlin@google.com>
It is only used for tests in the same package.

Signed-off-by: Michael Berlin <mberlin@google.com>
This way we avoid that a partial match would be a success as well.

Signed-off-by: Michael Berlin <mberlin@google.com>
@michael-berlin michael-berlin force-pushed the mikepaw.format-debug-urls branch from 02e7657 to 9dd560d Compare April 25, 2018 17:10
@michael-berlin
Copy link
Copy Markdown
Contributor

Thank you for addressing all the comments and the test :) The documentation of the feature looks very good now :)

I've rebased the code and fixed some minor nitpicks from my side (see the additional commits).

I'll merge this as soon as tests have passed.

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