Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Default value for sprintf interpolation (Feature) #4416

Open
breml opened this issue Jan 5, 2016 · 21 comments
Open

Default value for sprintf interpolation (Feature) #4416

breml opened this issue Jan 5, 2016 · 21 comments

Comments

@breml
Copy link
Contributor

breml commented Jan 5, 2016

In a lot places, Logstash allows to reference fields with the sprintf format(https://www.elastic.co/guide/en/logstash/current/event-dependent-configuration.html#sprintf). Unfortunately if the referenced field does not exist, sprintf defaults to leave the the reference untouched, which may lead to very unsatisfactory results. Therefore I suggest to allow a default value for the sprintf interpolation, e.g.

output {
  file {
    path => "/var/log/%{type:-default}.%{+yyyy.MM.dd.HH}"
  }
}

As delimiter between the field reference and the default value I suggest :- (taken from bash, unlikely to exist in a Logstash event field name).

@ph
Copy link
Contributor

ph commented Jan 5, 2016

I really like the idea of having a way to define a default value for a key in the string interpolation, this has come to my mind when I did the refactor and optimization of that part of the code.

I am not sure if the :- syntax is fine in that case, the other option that I could think of is this django-like template syntax %{type|default:mydefault}. Its a bit more explicit? @jordansissel @jsvd any opinion on this?

Another common request is a way to access environment variables through string interpolation, lets take both cases into consideration to come up with a the best syntax.

If i have an exported variable ABC="hello world" and I would like to access it via string interpolation maybe we could do the following

%{ENV|ABC|default:logs}%

What do you think? This would allow us to be flexible with new features and | is rarely used for keys?

@ph ph added the enhancement label Jan 5, 2016
@breml
Copy link
Contributor Author

breml commented Jan 6, 2016

I don't have any preference for the syntax. Actually I used the pipe (|) my self in the first drafts.

Regarding the environment variables have found #3944 which it self references logstash-plugins/logstash-filter-environment/pull/5. I think all the cases regarding environment variables, which could be addressed with the combined string interpolation syntax proposed by @ph, could also be solved by logstash-filter-environment. Therefore I suggest to not overload this pull request.

@ph
Copy link
Contributor

ph commented Jan 6, 2016

@breml I am +1 to not overload the PR, just wanted to make sure we could grow from it :)

@jordansissel
Copy link
Contributor

As a workaround, you can usually do this:


filter {
  ...
  if ![type] {
    mutate { add_field => { "type" => "default" } }
  }
}

output {
  file {
    path => "/var/log/%{type}.%{+yyyy.MM.dd.HH}"
  }
}

@jordansissel
Copy link
Contributor

I agree it would be nice to have more functionality in %{...} syntax, but I'm not sure how to approach it. There's more than just default values we may want to add (possible post-processing of values? I don't know). I'm hesitant to add more functionality into %{} for just one feature when there are lots of features we could add, and I'm worried about how this can grow in a way that is sensible for users.

@breml
Copy link
Contributor Author

breml commented Jan 7, 2016

@jordansissel: Thanks for the workaround, we will try this one. Up until now we used the blacklist_values feature of https://github.com/logstash-plugins/logstash-filter-prune to remove fields with %{...} content. But the above workaround is sure the better and cleaner solution.

@breml
Copy link
Contributor Author

breml commented Jan 7, 2016

I agree with @jordansissel, to have a clean syntax for a possible extension of the %{...} syntax, some more thinking is needed. I just proposed a solution for my actual problem, without the overall architecture in mind.

I am not sure, if we have to invent the wheel again. There are similar string interpolation features implemented in other places/languages.
bash, where I took the syntax from, is one of them. Maybe we should have a look at this solutions to get a better feeling, where a possible extension of the %{...} could lead us.

@breml
Copy link
Contributor Author

breml commented Jan 29, 2016

My PR #4417 is closed, based on the discussion in this issue. My question is: How do we precede with this one?

@andrewvc
Copy link
Contributor

@suyograo what was wrong with the code in #4417 ? I think it actual makes sense (though I'd like the delimiter to be |.

@suyograo
Copy link
Contributor

suyograo commented Feb 2, 2016

It looks like we haven't agreed on the syntax to define defaults based on above comments.

bash, where I took the syntax from, is one of them. Maybe we should have a look at this solutions to get a better feeling, where a possible extension of the %{...} could lead us.

I personally liked what @ph proposed. Its more explicit - %{type|default:mydefault}

@ph
Copy link
Contributor

ph commented Feb 2, 2016

I just reread the issue:

This comment from @jordansissel

possible post-processing of values

Like type coercion? We could implement it that way:

%{type|default:1,convert:integer}

not sure if it would be better if we support quoted text too, this would make the syntax more resilient to future changes.

%{type|default:"my new default"}

@suyograo
Copy link
Contributor

@breml How about sticking to the syntax %{field:"my new default"}. This is inline with the syntax we introduced for expanding environment variables: #3944. Thoughts?

@breml
Copy link
Contributor Author

breml commented Mar 12, 2016

@suyograo I thought about the same while reading about the environment variables feature but I did not find the time to update this issue until now. I just updated my PR #4417.
I defined the colon (:) to be the delimiter, but without the double quotes.

@vijayreddygudi
Copy link

@jordansissel @suyograo Any plans on adding this feature? Just wanted to get the idea as the issue hasn't been updated for a while now.

@jordansissel
Copy link
Contributor

I'm in favor of this feature, but we haven't worked on a design (and studied the possible negative impacts) beyond what you see in this issue.

@vijayreddygudi
Copy link

@jordansissel Thanks for the heads up.

@vbohata
Copy link

vbohata commented Dec 13, 2017

Is there any progress? This feature would be very useful.

@jordansissel
Copy link
Contributor

jordansissel commented Dec 13, 2017 via email

@dspruell-s01
Copy link

As a suggestion, curious to know if reasonable default that could be used here would be to simply interpolate the value of a missing field as an empty string. It is probably an improvement over placing the variable verbatim in the string in any case, but it's also similar to e.g. UNIX shell where accessing a variable that's not set returns an empty value.

$ echo $NOSUCHVAR; echo $((2+2))

4

If this for some reason isn't desirable, maybe it would be possible to enable this behavior with a configuration setting?

@brendongitx
Copy link

+1 for this.

@llermaly
Copy link

+1

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants