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

[TASK] add per-link statistics to influxdb #57

Merged
merged 10 commits into from
Sep 27, 2017
Merged

[TASK] add per-link statistics to influxdb #57

merged 10 commits into from
Sep 27, 2017

Conversation

corny
Copy link
Member

@corny corny commented Apr 18, 2017

closes #23

@corny corny force-pushed the link-stats branch 3 times, most recently from 01d77f4 to 890b15f Compare April 18, 2017 01:38
@genofire genofire added the wip label Apr 18, 2017
@corny corny requested a review from genofire April 18, 2017 09:54
runtime/nodes.go Outdated
// Update fields
node.Lastseen = now
node.Online = true
node.Neighbours = res.Neighbours
Copy link
Member

@genofire genofire Apr 18, 2017

Choose a reason for hiding this comment

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

👍 delete old values if they are not updated ... respond with get-request should answers with all requested parts (nodeinfo,statistics,neighbours)

@@ -77,6 +81,9 @@ func buildNodeStats(node *runtime.Node) (tags models.Tags, fields models.Fields)
batadv := 0
for _, batadvNeighbours := range neighbours.Batadv {
batadv += len(batadvNeighbours.Neighbours)
for neighbourID, link := range batadvNeighbours.Neighbours {
conn.insertLinkStatistics(stats.NodeID, neighbourID, link.Tq, time)
Copy link
Member

Choose a reason for hiding this comment

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

This is not the nodeid of the neighbour - it is the mac-address of the ínterface on this neighbour node

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that means it will be more complicated.

Copy link
Member

Choose a reason for hiding this comment

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

(or not so beautiful replace the stats.NodeID with the mac of the batman interface) <- 👎

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the MeshViewer able to deal with MAC addresses for links?

Copy link
Member

Choose a reason for hiding this comment

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

ATM no - he has no intention to deal with it - for it there are nodeid's

Copy link
Member

Choose a reason for hiding this comment

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

Es gibt also hier nur zwei lösungen:

  1. in der influxdb cachen von anderen nodes
  2. nodes global in runtime speichern und von anderen packages benutzbar (ggf. dort auch schon eine map mit den mac adressen einfügen)
  3. ... (ich weiß nicht alles, gibt es eine weitere Möglichkeit)

Wäre für option 2

Copy link
Member

Choose a reason for hiding this comment

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

@corny, würde es ja bauen ... wenn du mir sagst, 'wie'

Copy link
Member

Choose a reason for hiding this comment

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

also Variante 2

@genofire genofire changed the title Add per-link statistics [TASK] add per-link statistics to influxdb Apr 19, 2017
@genofire genofire force-pushed the link-stats branch 2 times, most recently from 2f23641 to b576e46 Compare September 2, 2017 14:49
@genofire
Copy link
Member

genofire commented Sep 2, 2017

Hihi, und es läuft auf dem server -> https://map.ffhb.de/#/en/map/60e327c79924-24a43ce7fe77
Beispiel

@genofire
Copy link
Member

genofire commented Sep 4, 2017

Würde gern die TQ umrechnen, in Prozent ... okay?

@genofire genofire added review and removed wip labels Sep 12, 2017
runtime/nodes.go Outdated
@@ -93,6 +88,10 @@ func (nodes *Nodes) Select(f func(*Node) bool) []*Node {
return result
}

func (nodes *Nodes) GetNodeIDByIface(mac string) string {
Copy link
Member Author

Choose a reason for hiding this comment

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

exported method Nodes.GetNodeIDByIface should have comment or be unexported

runtime/nodes.go Outdated
@@ -93,6 +88,10 @@ func (nodes *Nodes) Select(f func(*Node) bool) []*Node {
return result
}

func (nodes *Nodes) GetNodeIDByIface(mac string) string {
return nodes.iFaceToNodeID[mac]
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Kein Locking hier für eine exportiere Methode?

Copy link
Member

Choose a reason for hiding this comment

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

Ist doch "nur" lesend -> wird da locking benötigt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Na klar - oder sorgst du irgendwo dafür, dass nicht gleichzeitig geschrieben wird?

runtime/nodes.go Outdated
}
nodes.Unlock()
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Der Code dieser Methode ist nicht DRY. Ich räume sie auf.

@corny
Copy link
Member Author

corny commented Sep 13, 2017

In Prozent umrechen: Ja, bin ich für!

@genofire
Copy link
Member

Kann von mir aus nun ge squash+merge werden :)

@corny
Copy link
Member Author

corny commented Sep 14, 2017

Ohje, jetzt fällt's mir erst auf: Du hast das Einfügen der Link-Metriken in InsertNode() eingebaut. Bitte erstelle eine InsertLink()-Methode, welche die Linkdaten bekommt. Die Connect()-Methode muss unverändert bleiben.

Die Berechnung der Datenpunkte muss außerhalb der Datenbank-Module geschehen. Das ist ja nichts influxdb-Spezifisches. Separation of concerns mal wieder.

@corny corny force-pushed the link-stats branch 3 times, most recently from b4647af to 9675ea0 Compare September 14, 2017 10:50
@corny
Copy link
Member Author

corny commented Sep 14, 2017

Was passiert eigentlich, wenn mehr als einen Link zwischen zwei Knoten gibt? Bespielsweise im Falle von Mesh-On-LAN und gleichzeitigem Mesh über WLAN?

@@ -93,6 +90,27 @@ func (nodes *Nodes) Select(f func(*Node) bool) []*Node {
return result
}

// NodeLinks returns a list of links to known neighbours
func (nodes *Nodes) NodeLinks(node *Node) (result []Link) {
Copy link
Member

Choose a reason for hiding this comment

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

Gute Frage, da es blöd ist, wenn mehrere unterschiedliche TQs für "ein" Link zwischen zwei Knoten gemeldet werden.

Müsste man dies hier verarbeiten.
Policy best link wins?
Damit der LAN-Link TQ, den Mesh on Wifi schlägt?
Genauso wie ein 2.4 Ghz gegenüber ein 5 Ghz link

Copy link
Member Author

Choose a reason for hiding this comment

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

Ich habe mit @jplitza gesprochen. Wenn es mehrere Links gibt, sollten die Daten aller Links gespeichert werden. Also keine Abbildung von Interface-Adressen auf Node-IDs. Die Darstellung ist dann Sache des Mesh-Viewers.

Copy link
Member

Choose a reason for hiding this comment

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

Vorschlag, beides speichern, als Tag die MAC und die Nodeid von SRC und Target

Copy link
Member Author

Choose a reason for hiding this comment

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

Brauchen wir denn beides? Wie sehen die Abfragen aus, die wir machen?

Copy link
Member

Choose a reason for hiding this comment

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

Im Meshviewer:

https://grafana.bremen.freifunk.net/render/dashboard-solo/db/global-selectable?panelId=7&var-node={SOURCE_ID}&var-nodetolink={TARGET_ID}&from=now-86399s&width=650&height=350&theme=light

In Grafana als Influx-Query:

From Source:

SELECT mean("tq") FROM "link" WHERE ("source" =~ /^$node$/ AND "target" =~ /^$nodetolink$/) AND $timeFilter GROUP BY time($__interval) fill(null)

From Target:

SELECT mean("tq") FROM "link" WHERE ("target" =~ /^$node$/ AND "source" =~ /^$nodetolink$/) AND $timeFilter GROUP BY time($__interval) fill(null)

Min:

SELECT min("tq") FROM "link" WHERE ("target" =~ /^$node$/ AND "source" =~ /^$nodetolink$/ OR "source" =~ /^$node$/ AND "target" =~ /^$nodetolink$/) AND $timeFilter GROUP BY time($__interval) fill(null)

Copy link
Member

Choose a reason for hiding this comment

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

Diese kann man allerdings schlecht in Grafana oder anderen Auswertungstools für influxdb verwenden.
(Wenn der Meshviewer die MAC aus der JSON nutzt, doch eine Community kein Meshviewer nutzt, ist der Link total nutzlos.)

Copy link
Member

@jplitza jplitza Sep 20, 2017

Choose a reason for hiding this comment

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

Stimme @genofire da zu: Wenn das Grafana halbwegs self-contained sein soll, dann muss die NodeID (auch) mit rein. Und das sollte es meiner Meinung nach, um so etwas wie Dashboards zu Knoten oder zu Orten zu ermögilchen.

Und bei einer Retention Time von 7d stören uns die paar Daten doch nicht, oder?

Copy link
Member

Choose a reason for hiding this comment

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

Mich stört es nicht, das die MAC drin ist. Da es die vom Node ist - doch dein Satz, welches mit "Und ..."-Anfängt spricht doch eher für die NodeID.

Copy link
Member

Choose a reason for hiding this comment

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

Äh, sorry, ich war verwirrt. Ich meinte NodeID statt MAC-Adresse. Habs korrigiert.

Copy link
Member Author

Choose a reason for hiding this comment

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

Das sehe ich ein. Ich habe einen Commit hinzugefügt, damit jetzt Source-ID, Source-MAC, Target-ID und Target-MAC pro Link gespeichert werden.

@genofire
Copy link
Member

genofire commented Sep 24, 2017

<3 teste ich gleich - der Commit sieht so gut aus

@genofire genofire merged commit f465021 into master Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@FreifunkBremen FreifunkBremen deleted a comment from coveralls Sep 27, 2017
@genofire genofire deleted the link-stats branch September 27, 2017 11:56
@genofire
Copy link
Member

In database logging to add InsertLink() to file has no value.
(it is just a example Database type)

Whats is with Graphite @andir @MyIgel ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store TQ of links in InfluxDB
3 participants