munin-monitoring / contrib

Contributed stuff for munin (plugins, tools, etc...)
http://munin-monitoring.org
1.05k stars 683 forks source link

mtr100_ uses (%) in title #692

Closed leeclemens closed 7 years ago

leeclemens commented 8 years ago

The mtr100_ plugin hardcodes (%) as the source, for the graph_title. It seems to have always been this way, as mtr's output staring with HOST has always been excluded.

I believe it would be useful to display the source (although I haven't figured out a simple way to get the source IP for the route....as opposed to simply using mtr's output).

Was there a reason for this to be done this way initially? I'm going to work to update it, but wanted to raise the question as well.

leeclemens commented 8 years ago

Fixed in #693

leeclemens commented 8 years ago

Rewrote the command to use the same arguments as autoconf uses (also fixed unforeseen compat issue with Ubuntu).

sumpfralle commented 8 years ago

@leeclemens: your pull request was closed some months ago without merging. Do you still consider this bug report to be open? Would you mind preparing another pull request taking the discussion of #693 into account?

leeclemens commented 8 years ago

@sumpfralle I was asked a question and I answered. The pull was closed before I could respond - so I'm not sure I would call it a 'discussion'.

sumpfralle commented 8 years ago

I cannot read ssm's mind regarding the closing of the merge request, but I have to admit that I needed a good amount of guessing (even after your additional explanation in the pull request) to understand your goal. Reopening an issue is also an option, if you feel misunderstood.

My theory:

  1. You misunderstood the percent sign in the title: it is supposed to differentiate this plugin from another (not yet existing) plugin visualizing absolute ping times (ms) instead of ping time distribution (%). Thus the original title is technically perfectly fine.
  2. This misunderstanding lead you to the conclusion that it would be worthwhile to determine the local source IP of the traffic to the target
  3. Thus you found a reasonably well working way to achieve this. But this approach is hard to understand / easy to misunderstand if your goal is not clear.

Additionally picking the wording "SAME target" caused another bit of confusion, since this very exact spelling is used by iptables for a specific SNAT target implementation (see man iptables-extensions). This pushed me completely off-track :)

Sincerely I did not even get your point when I asked for the state of this bug report yesterday. Confusion everywhere - certainly no need for hard feelings. Have fun!

leeclemens commented 7 years ago

I'll admit I wasn't very excited for the interaction with the related PR. To be honest, I had forgotten this related Issue was still open after the PR was closed.

I felt I did answer ssm's question, but it was closed before I had the opportunity to (fine, if my answer was read afterwards). Lacking a followup question/comment, I figured my answer/response was dismissed or not read.

It's been a while since I considered this, since my update has been running great for me since and I haven't looked back.

The PR quite simply includes the source's hostname in the graph's title. I thought that was self-evident.

When viewing only the PNG, that crucial bit of information is lost. When viewing via Munin's web front, the source can be seen in the hyperlink above the graph (but not part of the PNG) or grouping. This came about when troubleshooting a network issue with a 3rd party and I found it easier to provide the raw PNG's than to screenshot and blur out, etc. Let's say three nodes, A, B, C...there were graphs for A->B, A->C, B->A, B->C, C->A, C->B. Simply providing the PNG's was a lot easier than renaming all of the saved files, etc.

To address your theories: 1) Re-reading my initial comment, yes, I may have misunderstood the purpose of (%) in the title or miscommunicated. As you said it differentiates from a "Not yet existing" plugin. My change could easily retain the (%) part - a trivial modification. I probably replaced it as it didn't seem to add any value (re Not yet existing). 2) Not a misunderstanding. That is exactly the goal of this change - to include the source as part of the graph's title (in the resultant PNG). This was the single goal of the PR. The PR's title was "Add source HOST to graph_title" :) 3) It includes the source in the graph's title, which is part of the resultant PNG. The PNG can therefore be viewed and both the source and target of the traceroute can be readily identified. This allows the graphs to be differentiated when multiple traces are performed against the same target/destination.

Regarding "SAME target": Munin creates the graphs based upon the source (since this plugin runs the traceroute and it is then associated with the source of the traceroute). I used the term target for the destination of the trace. Having the source in the graph title is useful when running multiple traces to the same target/destination.

I may have misunderstood, but it seemed the real question was the legitimacy of the change and not the methodology ("What, exactly, is the problem you are trying to solve?"). I used the same method to determine the source as autoconf does. And as the second commit shows, I ran in to interoperability issues with my first approach. If there is a better way, I would have expected a commit line comment, not a "close PR" reaction. Using mtr may be more expensive to determine the source, but it seemed appropriate given the plugin itself executes mtr.

If you don't want this change, you don't have to accept it :) I found it to be a valuable improvement and shared it. It was rejected. (again, I didn't realize this related Issue was still open.) If this further explanation helps understand the purpose and you agree, I'll be happy to continue.

sumpfralle commented 7 years ago

Fine - let us continue this discussion!

For a moment I thought, that your change was supposed to allow multiple graphs for different routes (via multiple interfaces) between a single source host and a single target host. But this would currently not be possible with the plugin, since no source IP can be specified (for mtr: --address). Thus your use case seems to refer to multiple source hosts that are using this plugin for their tracing their routes to the same single target. Is this correct?

In this case I would suggest to simply use hostname. Or does your use case really require a fully qualified domain name?

Besides this: I would feel uncomfortable to add a redundant information (the hostname of the munin node) to the graph. This is not done by any other plugin - thus it would be confusing for other users. What would you think about adding the local hostname to the list of trace hops with an undefined ("U") value (or zero)?

leeclemens commented 7 years ago

Yes, my use case came about from tracing between multiple hosts at multiple sites/availability zones, not via different local interfaces.

I've never argued for any specific method of putting some source identification in the graph, just that I found it useful for source identification to be in the graph.

I agree it is unlike other graphs to include the munin-node's own hostname in the graph's title. Due to the nature of this particular plugin, however, I believe this may be a case where it makes sense. They are all graphing the same thing (the destination) but from different perspectives (the source) and it is useful to compare these graphs against each other (you wouldn't necessarily compare disk graphs of the same disk from multiple source servers).

I don't believe I would call it redundant info, since it's not in the rendered/generated PNG graph image - but I do like your idea of including it as a value as opposed to putting it in the title.

tl;dr #771