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

Make graphite's /render/?.... urls copy-pastable #158

Merged
merged 2 commits into from Aug 11, 2014
Merged

Make graphite's /render/?.... urls copy-pastable #158

merged 2 commits into from Aug 11, 2014

Conversation

seveas
Copy link
Contributor

@seveas seveas commented Feb 24, 2013

Links to graphite images are completely unpastable to jabber/irc/mail as
clients mangle the humongous urls. These commits add a url shortener that can
shorten any graphite url from /what/ever/.... to /S/<base62_string>.

It can be used manually by prepending /s to the url path, and I added a button
to the graphite composer that does this for you.

@seveas
Copy link
Contributor Author

seveas commented Feb 24, 2013

Here's a screenshot of it in action:

shorturl

@SEJeff
Copy link
Member

SEJeff commented Feb 28, 2013

Haven't had a chance to test this yet, but I love the idea

@chjohnst
Copy link

Will give this a try tmo awesome stuff!

On Sunday, February 24, 2013, Dennis Kaarsemaker wrote:

Links to graphite images are completely unpastable to jabber/irc/mail as
clients mangle the humongous urls. These commits add a url shortener that
can
shorten any graphite url from /what/ever/.... to /S/.

It can be used manually by prepending /s to the url path, and I added a
button

to the graphite composer that does this for you.

You can merge this Pull Request by running

git pull https://github.com/seveas/graphite-web url_shortener

Or view, comment on, or merge it at:

#158
Commit Summary

  • Add url shortening views
  • Integrate the short url functionality with the composer

File Changes

Patch Links:

@seveas
Copy link
Contributor Author

seveas commented Feb 28, 2013

Thanks for considering. It's already proving pretty useful here, so I'm going to add links to create short urls to graphplot pages etc. as well.

@rolandow
Copy link

How do I get this button to fetch the URL in my Graphite? I only have the first three buttons shown in the screenshot, missing the last two.

@obfuscurity
Copy link
Member

I'm not 👎 on this, but it seems like odd bedfellows (to me) to add a URL shortener into Graphite proper. I understand your hesitancy to leak URLs outside of your organization, but there's probably a better solution imho.

@seveas
Copy link
Contributor Author

seveas commented Apr 25, 2013

It's certainly possible to use an external url shortener app, but it's the integration with the graphite UI that made it 1000% more useful for us than an external app. So my definition of 'better' is more integration :)

@seveas seveas closed this Apr 25, 2013
@seveas seveas reopened this Apr 25, 2013
@obfuscurity
Copy link
Member

I get the usability aspect of it, but it's a slippery slope if we start integrating bits that are tangentially helpful. By that logic we could add "send to" buttons for email, Campfire, Facebook, etc. Just because we can doesn't mean we should.

Again, I'm not 👎 on this, just trying to offer a voice of reason.

@SEJeff
Copy link
Member

SEJeff commented Apr 25, 2013

It does suck quite a bit to send graphite urls in emails or IM especially to users with brain dead email clients that get confused by urls with () in them. It also sucks in markdown. External URL shorteners no way, but a simple and integrated one would be much appreciated by some users.

When sending links to less technical users, this is very valuable.

@jmason
Copy link

jmason commented May 14, 2013

sniping from the bikesheds: fwiw, I've seen a similar feature in Amazon's internal graphite-like system to deal with unpasteably humungous URLs.

@devopsrick
Copy link

++interest in this feature! Any chance it will be merged?

@obfuscurity
Copy link
Member

@SEJeff Have you gotten a chance to test this? It seems the community has spoken. 😏

@lukevenediger
Copy link
Contributor

I've seen what @jmason is talking about: their graphing front-end auto-shortifies the link and displays it on the page. As an interim step, it would be awesome if the "Direct URL" button on the Dashboard gave me a complete link, as opposed to everything except the scheme and host. Extra points for auto-highlighting the whole line as it appears.

@SEJeff
Copy link
Member

SEJeff commented Jun 12, 2013

@obfuscurity I've not yet, but am going to shortly when I get home (travelling right now) at the end of the week. I'm in Boston at the moment

@seveas
Copy link
Contributor Author

seveas commented Jun 12, 2013

I just rebased the patches onto current master and pushed the result.

@seveas
Copy link
Contributor Author

seveas commented Jun 12, 2013

Just a datapoint as to how useful it is for us: in the 4 months since we added this, 666 short urls were created.

@obfuscurity
Copy link
Member

Clearly this is the devil's work at hand. Smite thine feature! 🔥

@jmason
Copy link

jmason commented Jun 13, 2013

\m/

@syepes
Copy link

syepes commented Jun 21, 2013

+1 This would be a great addition

@alistairking
Copy link

I'd also love to see this added. Was just about to code it myself when i saw this.

@alistairking
Copy link

I've been using this PR for a couple of months now and it has been really great - with one exception:
It seems that the default length for the URLField field type used in the model is 200 characters (https://docs.djangoproject.com/en/dev/ref/models/fields/#urlfield), a limit that I easily ran over with Graphite render URLs.

In fairness I only came across this problem after switching to a MySQL backend, so perhaps SQLite does not enforce this constraint?

To get around the problem I changed the url field type in url_shortener/models.py to "TextField" and then followed the alter table workaround described at http://code.djangoproject.com/ticket/2495 to trick MySQL and Django into playing nicely together when using a TextArea as part of a unique key.

I'm pretty new to django so I'm hoping there a better way to fix this.

@seveas
Copy link
Contributor Author

seveas commented Aug 23, 2013

Hmm, it's entirely possible that I did a manual alter table after syncdb, table definition here is:

url_shortener_link | CREATE TABLE `url_shortener_link` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `url` text NOT NULL,
  `date_submitted` datetime NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=1123 DEFAULT CHARSET=utf8 

@cbowman0
Copy link
Member

I cherry picked this into my branch and it appears to be usable after these two commits:
cbowman0@bb776c6
cbowman0@a5efba4

I will be testing more.

@obfuscurity
Copy link
Member

Sorry that this has languished for so long. If someone has the time and inclination to rebase this, I'd be happy to merge it.

Links to graphite images are completely unpastable to jabber/irc/mail as
clients mangle the humongous urls. These commits add a url shortener
that can shorten any graphite url from /what/ever/.... to
/S/something_short.

It can be used manually by prepending /s to any url. This will return a
shortened version of that url. Subsequent commits will integrate this
with the composer and dashboard, and maybe other places.
- The composer gets a button to shorten the URL to the current graph.
- The menu that appears when clicking on a graph in the dashboard
  gets a similar item.
@seveas
Copy link
Contributor Author

seveas commented Aug 10, 2014

I've pushed reworked commits based on todays master that include @cbowman0's changes.

@obfuscurity
Copy link
Member

Unless anyone has objections, I'd like to merge this in today or tomorrow. /cc @esc @brutasse @SEJeff

@SEJeff
Copy link
Member

SEJeff commented Aug 10, 2014

Please do, I for sure would use this functionality. I'd probably implement it differently, but this is entirely reasonable

@deniszh
Copy link
Member

deniszh commented Aug 10, 2014

Using that patch (backported to 0.9.12) in production, haven't any troubles, even on sqlite

obfuscurity added a commit that referenced this pull request Aug 11, 2014
Make graphite's /render/?.... urls copy-pastable
@obfuscurity obfuscurity merged commit b69aeb3 into graphite-project:master Aug 11, 2014
@jmason
Copy link

jmason commented Aug 11, 2014

awesome. thanks guys, looking forward to this ;)

@obfuscurity
Copy link
Member

Note that this is only in the master branch. I don't expect that we'd want to any significant features in the 0.9.x branch anytime soon; 0.9.13 is expected to be our last release from that branch. We'll focus on getting a 0.10 release from master after that.

@brutasse
Copy link
Member

It'd be nice to mention this change in the 0.10 release notes.

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