-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Feat: Allow filename as source to add custom label from __path__ #1013
Conversation
I think the approach I would rather take for this is to add |
@slim-bean Makes sense. Have made the changes. Kindly review. If this is fine, I will fix tests and modify the docs. |
Can you check if the full filename is part of labels coming in ? |
@cyriltovena I am just capturing like this |
Yeah I'm not too keen on breaking the configuration right now. @slim-bean should we just merge extracted with labels coming in ? |
Makes sense. Any approach is fine to me. Let know, I will push my changes accordingly. |
Yeah I'm going to have to agree on not wanting to make a breaking config change. I am debating if there are any disadvantages to having The most obvious would be naming collisions between data in the extracted map and labels, and which takes precedence. I am thinking because such collisions are easily controlled by the person creating the config, it would make sense if Or put another way, source would check for a matching label if nothing was found in the extractedMap. I am going to sleep on it and hopefully not change my mind tomorrow, but I'm thinking right now lets just keep source the way it is in config, but in the implementation first look in the extractedMap and if nothing is found, look for a label. |
have made changes accordingly. Kindly review. |
Can you update the documentation ? or give a hint about this new way of working for the source parameter ? Thanks ! |
Closing this PR in favour of #1122 |
what this PR does?
Allow filename as source to add custom label from path
Which issue(s) this PR fixes:
Fixes #775
@cyriltovena @sandlis I am not sure if this change is a good way to handle the issue described in #775 but I couldn't think of anything better than this. Let me know your thoughts.