Is this comment still relevant?

Hmm, might still be relevant
We're not sure if TYPE text is optional yet
if it is how do we handle it

This is nice, but it might be too hidden logic from the parser caller perspective. Perhaps less surprising option is to offer a choice on parser constructor for this? Another reason is flexibility, perhaps some parser users would like to ALWAYS skip CT series even without calling CreatedTimestamp? Or opposite, they want to preserve those even when calling CreatedTimestamp.

If it's simple to implement for us, perhaps it makes sense more to use constructor for that decision and make it orthogonal to CreatedTimestamp method use.

NOTE: Likely in Prometheus we will set this to True when CT is enabled, but in future there might option to always set it to true (although one could use relabelling for this)

To be addressed?

About this, essentially whenever we call an OM parser we want skipCT to always be true. The only case the parser will not skip CT is when it is a deepCopy

In that sense I'm not too sure if it makes sense (I could be wrong) to give to option to skipCT via a constructor. This is moreso just a way to track which parser is a deepCopy and which ins't especially when we're calling parser.Next()

I'm failing to understand why this isn't needed as well 😬. From our discussions, I understood that we don't want to add _created lines as a separate new metric. This means that the original parser needs to skip _created lines when calling Next(), but the ephemeral parser in CreatedTimestamp() needs to return it

update to this can be found in this comment: #14356 (comment)

I'm failing to understand why this isn't needed as well 😬 From our discussions, I understood that we don't want to add _created lines as a separate new metric.

Not sure what @ArthurSens mean by "this" now. I only refer to the logic how this field is set. There is quite large and complex explanation in commentry how this is set. I feel it would be easier if we kill this and simply allow constructor to set it. In fact textparse.WithSkipCT was already implemented for other reasons, so we have it. My proposal is to:

• keep some default (skip = false?)
• ensure that option as we have now e.g textparse.WithOMParserCTSeriesSkipped()
• don't change this variable anywhere else (ofc pass true to deep copied for CT logic)

I think this is already implemented by @Maniktherana by a quick look, just I added suggestions below for default and option name 🤗 Thanks!

To be addressed?

Why public?

its being used in promparse_test so I thought it would be public in this case

Why? This appears to be what the code above already does.

this will be removed in my PR here

But this looks like its a relic from an earlier implementation in this PR that I forgot to remove: #14356 (comment)