Skip to content

Conversation

jtlisi
Copy link
Contributor

@jtlisi jtlisi commented Apr 22, 2020

What this PR does:

  • Ensure the Ruler can properly handle rule Namespaces with special characters. E.G. /+<>?

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@jtlisi jtlisi changed the title handle forward slashes in ruler mapper/manager Handle forward slashes in ruler mapper/manager Apr 22, 2020
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the upgrade path. I assume there are already existing rule files. How do we handle those?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
level.Warn(m.logger).Log("msg", "unable to remove rule file on disk", "file", existingFile.Name(), "err", err)
level.Warn(m.logger).Log("msg", "unable to remove rule file on disk", "file", fullFileName.Name(), "err", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These aren't rule files meant for the storage and persistence of rules. They are temporary files that are loaded by the upstream prometheus manager code for evaluation. They are essentially stateless so no upgrade path is required.

CHANGELOG.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be considered a CHANGE, mentioning that when we store rules in the filesystem we now base64 encode the filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

The main downside of this approach is that the encoded filename is unreadable and makes debugging harder. An alternative may be using url.PathEscape() (as far as % character is not a problem). Same examples:

  • Hello > Hello
  • Hello world > Hello%20world
  • a/b > a%2Fb

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this better. This means the existing file names are same as well.

Copy link
Contributor Author

@jtlisi jtlisi Apr 27, 2020

Choose a reason for hiding this comment

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

This would only work for POSIX, no Windows. Will that be an issue?

@jtlisi jtlisi force-pushed the 20200421_slash_handling_ruler branch from d40e14f to 1afd8e2 Compare April 27, 2020 17:35
CHANGELOG.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to bother you on this, but I think that reading the changelog entry it's not clear why it's a CHANGE. Could you clarify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to give it more thought. I'm starting to think a changelog entry is not needed or if anything it should be labeled a BUGFIX since it aligns Cortex with expected behavior.

@jtlisi jtlisi force-pushed the 20200421_slash_handling_ruler branch from 1afd8e2 to 52ae064 Compare April 28, 2020 16:02
Signed-off-by: Jacob Lisi <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @jtlisi for addressing my feedback. LGTM!

Signed-off-by: Jacob Lisi <[email protected]>
@jtlisi jtlisi merged commit d94cdce into cortexproject:master Apr 30, 2020
@jtlisi jtlisi deleted the 20200421_slash_handling_ruler branch April 30, 2020 01:15
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.

4 participants