Correct nh sample span structure and parsing#1082
Correct nh sample span structure and parsing#1082csmarchbanks merged 8 commits intoprometheus:masterfrom
Conversation
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
…le-span-structure Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
… be None Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
There was a problem hiding this comment.
nit: i'd prefer early return instead of exception handling
if spans_name not in spans:
return None
There was a problem hiding this comment.
That _text is Hungarian notation :)
| pos_spans_text = spans[spans_name] | |
| pos_spans = [] | |
| for start, end in pos_spans_text: | |
| for start, end in spans[spans_name]: |
There was a problem hiding this comment.
Adding this here makes the code a little inconsistent, you do the text->data conversion here for spans, but in a helper function for the deltas. Let's move this into the helper function.
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
| ) | ||
|
|
||
|
|
||
| def _compose_spans(span_matches, spans_name): |
There was a problem hiding this comment.
nit: might want to add a comment what this does, because the list comprehension takes a minute to understand :)
There was a problem hiding this comment.
Yes, that makes sense! I've also added comments to the other less complicated function, while I was at it :D
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
Also 👍 though I will give you a moment if you want to add a comment, I agree it would be nice.
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
3 participants
While working on the exposition part of the issue prometheus/OpenMetrics#279, I realized I made a mistake in the code relative to the NH spans and deltas parsing, in that I was not taking into account that span lists have no fixed length, span lists can be absent and that deltas can consequently also not be there. I thought I’d get this right before continuing on the rest of the work.