parser: ensure samples are of type Sample

[crepererum]

No description provided.

[brian-brazil]

Why does assertEqual not work?

[crepererum]

@brian-brazil because Sample is a subtype of tuple and therefore the equality treats them as equal. But named attributes (like sample.value) do not work for tuples so I think it's important to be consistent here.

[brian-brazil]

If you want to test for the named attributes, then I suggest testing for them as that's more in line with Python's duck typing.

[crepererum]

I have 3 arguments against "testing the ducktype" here:

Maintainability

Well, then we have to keep the tests and the Sample definition in-sync. Possible, but not nice.

Robustness / Clean Code

Not sure how sane that would be. As a user, I basically expect:

• a tuple
• named attributes

which is exactly the definition of Sample. Sure, somebody could in theory come up w/ another type that fulfills this, but since we're testing our own library here. I would say we follow ducktyping in the following way:

Be conservative in what you do, be liberal in what you accept from others (often reworded as "Be conservative in what you send, be liberal in what you accept").

(Robustness principle)

Type Zoo

We have a single library here and I think this single library should use a single type for that dedicated purpose (= representing samples). This partly contradicts ducktyping (in the provider/return part) but is nice for contributors to understand the library and for users to grasp the concepts. Also, it would make type annotations way easier in the future (in case we want to walk that path). Notice that consistent types make downstream libraries also simpler (since they can use type annotations) and enables proper IDE support.

If this doesn't convince you, I can change the tests. Just let me know and thank you for your time and effort.

[brian-brazil]

I'd prefer to keep the tests simple.

[crepererum]

Then I don't understand. Your proposed solution to just test (all) named attributes is not simpler than the isinstance test. It's more code and more maintenance burden.

[brian-brazil]

I'm suggesting using assertEquals, and you want to check specific types are right verify that they have one of the named attributes.

[crepererum]

Alright then. Even though I very much disagree with you, I'll change the test when I'm back in the office (Monday).

[crepererum]

done

[brian-brazil]

In one or two tests is enough.

[crepererum]

Not sure this is enough. The problem is that the return type was Sample (aka a name tuple) for some and a normal tuple for others. Or in other words: the parser returned inconsistent types. That depends on whether the fixed code path was taken or not. And the code path is taken iff the metric type is counter and the name does not end with _total. As a user, this is pretty much confusing. To prevent this behavior in the future, I've added consistence checks to all tests.

[brian-brazil]

Can you add a comment as to why you're doing this?

[brian-brazil]

That's a bit lengthy, you only need to say that you're checking the type is correct.

[crepererum]

done

[nichenke]

I wanted to chime in with a 'nice catch' on this and add my vote for moving this to a release as soon as reasonable.
FWIW: our case is parsing metrics sent by other libraries (java, scala) that don't add _total by default to Counters. We detected this as generate_latest blew up on AttributeError: 'tuple' object has no attribute 'labels' etc.

[brian-brazil]

Thanks!