Do not override some placeholders which are built from records

lib/fluent/plugin/prometheus/placeholder_expander.rb

@@ -2,6 +2,10 @@ module Fluent
   module Plugin
     module Prometheus
       class ExpandBuilder
+        # ${tag}, ${tag_parts[...]}, ${tag_prefix[...]} and ${tag_suffix[...]}
+        # must be built from the tag only.
+        TAG_DERIVED_PLACEHOLDER = /\A\$\{tag(_parts|_prefix|_suffix)?(\[[^\]]*\])?\}\z/.freeze
+
         def self.build(placeholder, log:)
           new(log: log).build(placeholder)
         end
@@ -12,6 +16,7 @@ def initialize(log:)
 
         def build(placeholder_values)
           placeholders = {}
+          tag_placeholders = {}
           placeholder_values.each do |key, value|
             case value
             when Array
@@ -26,13 +31,22 @@ def build(placeholder_values)
               end
             else
               if key == 'tag'
-                placeholders.merge!(build_tag(value))
+                tag_placeholders = build_tag(value)
               else
                 placeholders["${#{key}}"] = value
               end
             end
           end
 
+          # A record may have an array named "tag_parts", or a key named
+          # "tag_parts[0]". Both make the same placeholder as the tag does.
+          # merge! is not enough here, because a record can also use an index
+          # which build_tag does not make, such as ${tag_parts[3]} for a tag
+          # of 3 parts, or ${tag_prefix[-1]}. So remove them first, then such
+          # a placeholder stays unknown.
+          placeholders.delete_if { |k, _| TAG_DERIVED_PLACEHOLDER.match?(k) }
+          placeholders.merge!(tag_placeholders)
+
           Fluent::Plugin::Prometheus::ExpandBuilder::PlaceholderExpander.new(@log, placeholders)
         end
 

spec/fluent/plugin/filter_prometheus_spec.rb

@@ -45,4 +45,73 @@
 
     it_behaves_like 'instruments record'
   end
+
+  # a label which uses the tag must be expanded with the tag, not with the
+  # record.
+  describe 'a record which has a tag placeholder name' do
+    let(:config) {
+      BASE_CONFIG + %(
+        <metric>
+          name tagged
+          type counter
+          desc Something foo.
+          key foo
+          <labels>
+            part ${tag_parts[0]}
+          </labels>
+        </metric>
+      )
+    }
+
+    it 'labels the metric with the tag' do
+      driver.run(default_tag: tag) do
+        driver.feed(event_time, {'tag' => 'forged.tag', 'foo' => 1, 'tag_parts' => ['forged']})
+      end
+      # even though tag_parts exists in the record, it should not be taken as "part".
+      expect(registry.get(:tagged).values.keys).to eq([{part: 'prometheus'}])
+    end
+
+    it 'labels the metric with the tag, not with the record key' do
+      driver.run(default_tag: tag) do
+        driver.feed(event_time, {'tag' => 'forged.tag', 'foo' => 1, 'tag_parts[0]' => 'forged'})
+      end
+      # the record has a key named "tag_parts[0]", but the tag must win.
+      expect(registry.get(:tagged).values.keys).to eq([{part: 'prometheus'}])
+    end
+  end
+
+  # a record must not fill an index which the tag does not have.
+  describe 'a record which uses an index out of the tag' do
+    let(:config) {
+      BASE_CONFIG + %(
+        <metric>
+          name out_of_range
+          type counter
+          desc Something foo.
+          key foo
+          <labels>
+            part ${tag_parts[2]}
+            prefix ${tag_prefix[-1]}
+            suffix ${tag_suffix[-1]}
+          </labels>
+        </metric>
+      )
+    }
+
+    it 'leaves the label unexpanded' do
+      driver.run(default_tag: tag) do
+        driver.feed(event_time, {
+                      'foo' => 1,
+                      'tag_parts[2]' => 'forged',
+                      'tag_prefix[-1]' => 'forged',
+                      'tag_suffix[-1]' => 'forged',
+                    })
+      end
+      # the tag "prometheus.test" has 2 parts, and tag_prefix and tag_suffix
+      # have no negative index. So these labels must not be expanded.
+      expect(registry.get(:out_of_range).values.keys).to eq(
+        [{part: '${tag_parts[2]}', prefix: '${tag_prefix[-1]}', suffix: '${tag_suffix[-1]}'}]
+      )
+    end
+  end
 end

spec/fluent/plugin/prometheus/placeholder_expander_spec.rb

@@ -50,6 +50,94 @@
         expander.expand('${hostname}')
       end
 
+      # tag_parts, tag_prefix and tag_suffix must be expanded with the tag,
+      # not with "forged"
+      context 'with a value named after a tag placeholder' do
+        let(:forged_placeholder) do
+          {
+            'tag' => '1.2.3',
+            'tag_parts' => %w[forged forged forged],
+            'tag_prefix' => %w[forged forged forged],
+            'tag_suffix' => %w[forged forged forged],
+          }
+        end
+
+        it 'expands the placeholders with the tag' do
+          expander = builder.build(forged_placeholder)
+
+          expect(expander.expand('${tag_parts[0]}.${tag_parts[1]}.${tag_parts[2]}')).to eq('1.2.3')
+          expect(expander.expand('${tag_parts[-3]}.${tag_parts[-2]}.${tag_parts[-1]}')).to eq('1.2.3')
+          expect(expander.expand('${tag_prefix[0]},${tag_prefix[1]},${tag_prefix[2]}')).to eq('1,1.2,1.2.3')
+          expect(expander.expand('${tag_suffix[0]},${tag_suffix[1]},${tag_suffix[2]}')).to eq('3,2.3,1.2.3')
+        end
+
+        it 'does not expand an index which the tag does not have' do
+          # tag_prefix and tag_suffix have no negative index, so they are kept
+          # as they are. This checks that the record does not fill them.
+          expander = builder.build(forged_placeholder)
+
+          expect(expander.expand('${tag_prefix[-1]}')).to eq('${tag_prefix[-1]}')
+          expect(expander.expand('${tag_suffix[-1]}')).to eq('${tag_suffix[-1]}')
+
+          # the tag has 3 parts, so the 4th value of the record is out of it.
+          longer = forged_placeholder.merge('tag_parts' => %w[forged forged forged forged])
+          expander = builder.build(longer)
+
+          expect(expander.expand('${tag_parts[3]}')).to eq('${tag_parts[3]}')
+          expect(expander.expand('${tag_parts[-4]}')).to eq('${tag_parts[-4]}')
+        end
+      end
+
+      # a record may also have a key named "tag_parts[0]", which makes the
+      # same placeholder as the tag
+      context 'with a value whose key is a tag placeholder' do
+        it 'expands the placeholders with the tag' do
+          spelled = {
+            'tag' => '1.2.3',
+            'tag_parts[0]' => 'forged',
+            'tag_prefix[0]' => 'forged',
+            'tag_suffix[0]' => 'forged',
+          }
+          expander = builder.build(spelled)
+
+          expect(expander.expand('${tag_parts[0]}')).to eq('1')
+          expect(expander.expand('${tag_prefix[0]}')).to eq('1')
+          expect(expander.expand('${tag_suffix[0]}')).to eq('3')
+        end
+
+        it 'does not expand an index which the tag does not have' do
+          spelled = {
+            'tag' => '1',
+            'tag_parts[1]' => 'forged',
+            'tag_parts[-2]' => 'forged',
+            'tag_prefix[-1]' => 'forged',
+            'tag_suffix[-1]' => 'forged',
+          }
+          expander = builder.build(spelled)
+
+          expect(expander.expand('${tag_parts[1]}')).to eq('${tag_parts[1]}')
+          expect(expander.expand('${tag_parts[-2]}')).to eq('${tag_parts[-2]}')
+          expect(expander.expand('${tag_prefix[-1]}')).to eq('${tag_prefix[-1]}')
+          expect(expander.expand('${tag_suffix[-1]}')).to eq('${tag_suffix[-1]}')
+        end
+
+        it 'keeps the tag itself' do
+          spelled = {
+            'tag' => '1.2.3',
+            'tag_parts' => 'forged',
+            'tag_prefix' => 'forged',
+            'tag_suffix' => 'forged',
+          }
+          expander = builder.build(spelled)
+
+          expect(expander.expand('${tag}')).to eq('1.2.3')
+          # these are not built from the tag, so they are kept as they are
+          expect(expander.expand('${tag_parts}')).to eq('${tag_parts}')
+          expect(expander.expand('${tag_prefix}')).to eq('${tag_prefix}')
+          expect(expander.expand('${tag_suffix}')).to eq('${tag_suffix}')
+        end
+      end
+
       context 'when not found placeholder' do
         it 'prints wanring log and as it is' do
           expect(log).to receive(:warn).with('unknown placeholder `${tag_prefix[100]}` found').once
@@ -87,6 +175,14 @@
         expect(expander.expand('${tag_suffix[0]}.${tag_suffix[1]}.${tag_suffix[2]}', dynamic_placeholders: dynamic_placeholder)).to eq('3.2.3.1.2.3')
       end
 
+      it 'expands the placeholders with the dynamic tag' do
+        forged = static_placeholder.merge('tag_parts' => %w[forged forged forged])
+        expander = builder.build(forged)
+
+        expect(expander.expand('${tag_parts[0]}.${tag_parts[1]}.${tag_parts[2]}',
+                               dynamic_placeholders: dynamic_placeholder)).to eq('1.2.3')
+      end
+
       it 'does not create expander twice if given the same placeholder' do
         builder # cached before mock