Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore(inputs.modbus): Add device or controller information to error messages #16114

Merged
merged 1 commit into from
Nov 4, 2024
Merged

chore(inputs.modbus): Add device or controller information to error messages #16114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 25 additions & 29 deletions plugins/inputs/modbus/configuration_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1722,7 +1722,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: empty field name in request for slave 1",
errormsg: "empty field name in request for slave 1",
},
{
name: "invalid byte-order (coil)",
Expand All @@ -1733,7 +1733,7 @@ func TestRequestFail(t *testing.T) {
RegisterType: "coil",
},
},
errormsg: "configuration invalid: unknown byte-order \"AB\"",
errormsg: "unknown byte-order \"AB\"",
},
{
name: "duplicate fields (coil)",
Expand All @@ -1754,7 +1754,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"coil-0\" duplicated in measurement \"modbus\" (slave 1/\"coil\")",
errormsg: "field \"coil-0\" duplicated in measurement \"modbus\" (slave 1/\"coil\")",
},
{
name: "duplicate fields multiple requests (coil)",
Expand Down Expand Up @@ -1784,7 +1784,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"coil-0\" duplicated in measurement \"foo\" (slave 1/\"coil\")",
errormsg: "field \"coil-0\" duplicated in measurement \"foo\" (slave 1/\"coil\")",
},
{
name: "invalid byte-order (discrete)",
Expand All @@ -1795,7 +1795,7 @@ func TestRequestFail(t *testing.T) {
RegisterType: "discrete",
},
},
errormsg: "configuration invalid: unknown byte-order \"AB\"",
errormsg: "unknown byte-order \"AB\"",
},
{
name: "duplicate fields (discrete)",
Expand All @@ -1816,7 +1816,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"discrete-0\" duplicated in measurement \"modbus\" (slave 1/\"discrete\")",
errormsg: "field \"discrete-0\" duplicated in measurement \"modbus\" (slave 1/\"discrete\")",
},
{
name: "duplicate fields multiple requests (discrete)",
Expand Down Expand Up @@ -1846,7 +1846,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"discrete-0\" duplicated in measurement \"foo\" (slave 1/\"discrete\")",
errormsg: "field \"discrete-0\" duplicated in measurement \"foo\" (slave 1/\"discrete\")",
},
{
name: "invalid byte-order (holding)",
Expand All @@ -1857,7 +1857,7 @@ func TestRequestFail(t *testing.T) {
RegisterType: "holding",
},
},
errormsg: "configuration invalid: unknown byte-order \"AB\"",
errormsg: "unknown byte-order \"AB\"",
},
{
name: "invalid field name (holding)",
Expand All @@ -1872,7 +1872,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: empty field name in request for slave 1",
errormsg: "empty field name in request for slave 1",
},
{
name: "invalid field input type (holding)",
Expand All @@ -1888,7 +1888,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "cannot process configuration: initializing field \"holding-0\" failed: invalid input datatype \"\" for determining field length",
errormsg: "initializing field \"holding-0\" failed: invalid input datatype \"\" for determining field length",
},
{
name: "invalid field output type (holding)",
Expand All @@ -1906,7 +1906,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: `configuration invalid: unknown output data-type "UINT8" for field "holding-0"`,
errormsg: `unknown output data-type "UINT8" for field "holding-0"`,
},
{
name: "duplicate fields (holding)",
Expand All @@ -1927,7 +1927,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"holding-0\" duplicated in measurement \"modbus\" (slave 1/\"holding\")",
errormsg: "field \"holding-0\" duplicated in measurement \"modbus\" (slave 1/\"holding\")",
},
{
name: "duplicate fields multiple requests (holding)",
Expand Down Expand Up @@ -1957,7 +1957,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"holding-0\" duplicated in measurement \"foo\" (slave 1/\"holding\")",
errormsg: "field \"holding-0\" duplicated in measurement \"foo\" (slave 1/\"holding\")",
},
{
name: "invalid byte-order (input)",
Expand All @@ -1968,7 +1968,7 @@ func TestRequestFail(t *testing.T) {
RegisterType: "input",
},
},
errormsg: "configuration invalid: unknown byte-order \"AB\"",
errormsg: "unknown byte-order \"AB\"",
},
{
name: "invalid field name (input)",
Expand All @@ -1983,7 +1983,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: empty field name in request for slave 1",
errormsg: "empty field name in request for slave 1",
},
{
name: "invalid field input type (input)",
Expand All @@ -1999,7 +1999,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "cannot process configuration: initializing field \"input-0\" failed: invalid input datatype \"\" for determining field length",
errormsg: "initializing field \"input-0\" failed: invalid input datatype \"\" for determining field length",
},
{
name: "invalid field output type (input)",
Expand All @@ -2017,7 +2017,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: `configuration invalid: unknown output data-type "UINT8" for field "input-0"`,
errormsg: `unknown output data-type "UINT8" for field "input-0"`,
},
{
name: "duplicate fields (input)",
Expand All @@ -2038,7 +2038,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"input-0\" duplicated in measurement \"modbus\" (slave 1/\"input\")",
errormsg: "field \"input-0\" duplicated in measurement \"modbus\" (slave 1/\"input\")",
},
{
name: "duplicate fields multiple requests (input)",
Expand Down Expand Up @@ -2068,7 +2068,7 @@ func TestRequestFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: field \"input-0\" duplicated in measurement \"foo\" (slave 1/\"input\")",
errormsg: "field \"input-0\" duplicated in measurement \"foo\" (slave 1/\"input\")",
},
}

Expand All @@ -2082,9 +2082,7 @@ func TestRequestFail(t *testing.T) {
}
plugin.Requests = tt.requests

err := plugin.Init()
require.Error(t, err)
require.Equal(t, tt.errormsg, err.Error())
require.ErrorContains(t, plugin.Init(), tt.errormsg)
require.Empty(t, plugin.requests)
})
}
Expand Down Expand Up @@ -2258,7 +2256,7 @@ func TestRequestEmptyFields(t *testing.T) {
},
}
err := modbus.Init()
require.EqualError(t, err, `configuration invalid: found request section without fields`)
require.ErrorContains(t, err, `found request section without fields`)
}

func TestRequestMultipleSlavesOneFail(t *testing.T) {
Expand Down Expand Up @@ -2357,7 +2355,7 @@ func TestRequestMultipleSlavesOneFail(t *testing.T) {
actual := acc.GetTelegrafMetrics()
testutil.RequireMetricsEqual(t, expected, actual, testutil.IgnoreTime(), testutil.SortMetrics())
require.Len(t, acc.Errors, 1)
require.ErrorContains(t, acc.FirstError(), "slave 2: modbus: exception '11' (gateway target device failed to respond)")
require.ErrorContains(t, acc.FirstError(), `slave 2 on controller "tcp://localhost:1502": modbus: exception '11' (gateway target device failed to respond)`)
}

func TestRequestOptimizationShrink(t *testing.T) {
Expand Down Expand Up @@ -3002,7 +3000,7 @@ func TestRequestOptimizationMaxExtraRegisterFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: optimization_max_register_fill has to be between 1 and 125",
errormsg: "optimization_max_register_fill has to be between 1 and 125",
},
{
name: "MaxExtraRegister too small",
Expand All @@ -3022,7 +3020,7 @@ func TestRequestOptimizationMaxExtraRegisterFail(t *testing.T) {
},
},
},
errormsg: "configuration invalid: optimization_max_register_fill has to be between 1 and 125",
errormsg: "optimization_max_register_fill has to be between 1 and 125",
}}

for _, tt := range tests {
Expand All @@ -3035,9 +3033,7 @@ func TestRequestOptimizationMaxExtraRegisterFail(t *testing.T) {
}
plugin.Requests = tt.requests

err := plugin.Init()
require.Error(t, err)
require.Equal(t, tt.errormsg, err.Error())
require.ErrorContains(t, plugin.Init(), tt.errormsg)
require.Empty(t, plugin.requests)
})
}
Expand Down
38 changes: 19 additions & 19 deletions plugins/inputs/modbus/modbus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (m *Modbus) Init() error {
}

if m.Retries < 0 {
return errors.New("retries cannot be negative")
return fmt.Errorf("retries cannot be negative in device %q", m.Name)
}

// Determine the configuration style
Expand All @@ -157,23 +157,23 @@ func (m *Modbus) Init() error {
m.ConfigurationPerMetric.logger = m.Log
cfg = &m.ConfigurationPerMetric
default:
return fmt.Errorf("unknown configuration type %q", m.ConfigurationType)
return fmt.Errorf("unknown configuration type %q in device %q", m.ConfigurationType, m.Name)
}

// Check and process the configuration
if err := cfg.Check(); err != nil {
return fmt.Errorf("configuration invalid: %w", err)
return fmt.Errorf("configuration invalid for device %q: %w", m.Name, err)
}

r, err := cfg.Process()
if err != nil {
return fmt.Errorf("cannot process configuration: %w", err)
return fmt.Errorf("cannot process configuration for device %q: %w", m.Name, err)
}
m.requests = r

// Setup client
if err := m.initClient(); err != nil {
return fmt.Errorf("initializing client failed: %w", err)
return fmt.Errorf("initializing client failed for controller %q: %w", m.Controller, err)
}
for slaveID, rqs := range m.requests {
var nHoldingRegs, nInputsRegs, nDiscreteRegs, nCoilRegs uint16
Expand All @@ -195,23 +195,23 @@ func (m *Modbus) Init() error {
nCoilRegs += r.length
nCoilFields += len(r.fields)
}
m.Log.Infof("Got %d request(s) touching %d holding registers for %d fields (slave %d)",
len(rqs.holding), nHoldingRegs, nHoldingFields, slaveID)
m.Log.Infof("Got %d request(s) touching %d holding registers for %d fields (slave %d) on device %q",
len(rqs.holding), nHoldingRegs, nHoldingFields, slaveID, m.Name)
for i, r := range rqs.holding {
m.Log.Debugf(" #%d: @%d with length %d", i+1, r.address, r.length)
}
m.Log.Infof("Got %d request(s) touching %d inputs registers for %d fields (slave %d)",
len(rqs.input), nInputsRegs, nInputsFields, slaveID)
m.Log.Infof("Got %d request(s) touching %d inputs registers for %d fields (slave %d) on device %q",
len(rqs.input), nInputsRegs, nInputsFields, slaveID, m.Name)
for i, r := range rqs.input {
m.Log.Debugf(" #%d: @%d with length %d", i+1, r.address, r.length)
}
m.Log.Infof("Got %d request(s) touching %d discrete registers for %d fields (slave %d)",
len(rqs.discrete), nDiscreteRegs, nDiscreteFields, slaveID)
m.Log.Infof("Got %d request(s) touching %d discrete registers for %d fields (slave %d) on device %q",
len(rqs.discrete), nDiscreteRegs, nDiscreteFields, slaveID, m.Name)
for i, r := range rqs.discrete {
m.Log.Debugf(" #%d: @%d with length %d", i+1, r.address, r.length)
}
m.Log.Infof("Got %d request(s) touching %d coil registers for %d fields (slave %d)",
len(rqs.coil), nCoilRegs, nCoilFields, slaveID)
m.Log.Infof("Got %d request(s) touching %d coil registers for %d fields (slave %d) on device %q",
len(rqs.coil), nCoilRegs, nCoilFields, slaveID, m.Name)
for i, r := range rqs.coil {
m.Log.Debugf(" #%d: @%d with length %d", i+1, r.address, r.length)
}
Expand All @@ -230,15 +230,15 @@ func (m *Modbus) Gather(acc telegraf.Accumulator) error {
for slaveID, requests := range m.requests {
m.Log.Debugf("Reading slave %d for %s...", slaveID, m.Controller)
if err := m.readSlaveData(slaveID, requests); err != nil {
acc.AddError(fmt.Errorf("slave %d: %w", slaveID, err))
acc.AddError(fmt.Errorf("slave %d on controller %q: %w", slaveID, m.Controller, err))
var mbErr *mb.Error
if !errors.As(err, &mbErr) || mbErr.ExceptionCode != mb.ExceptionCodeServerDeviceBusy {
m.Log.Debugf("Reconnecting to %s...", m.Controller)
if err := m.disconnect(); err != nil {
return fmt.Errorf("disconnecting failed: %w", err)
return fmt.Errorf("disconnecting failed for controller %q: %w", m.Controller, err)
}
if err := m.connect(); err != nil {
return fmt.Errorf("slave %d: connecting failed: %w", slaveID, err)
return fmt.Errorf("slave %d on controller %q: connecting failed: %w", slaveID, m.Controller, err)
}
}
continue
Expand Down Expand Up @@ -319,7 +319,7 @@ func (m *Modbus) initClient() error {
handler.Logger = tracelog
m.handler = handler
default:
return fmt.Errorf("invalid transmission mode %q for %q", m.TransmissionMode, u.Scheme)
return fmt.Errorf("invalid transmission mode %q for %q on device %q", m.TransmissionMode, u.Scheme, m.Name)
}
case "", "file":
path := filepath.Join(u.Host, u.Path)
Expand Down Expand Up @@ -362,7 +362,7 @@ func (m *Modbus) initClient() error {
}
m.handler = handler
default:
return fmt.Errorf("invalid transmission mode %q for %q", m.TransmissionMode, u.Scheme)
return fmt.Errorf("invalid transmission mode %q for %q on device %q", m.TransmissionMode, u.Scheme, m.Name)
}
default:
return fmt.Errorf("invalid controller %q", m.Controller)
Expand Down Expand Up @@ -408,7 +408,7 @@ func (m *Modbus) readSlaveData(slaveID byte, requests requestSet) error {
}

// Wait some time and try again reading the slave.
m.Log.Infof("Device busy! Retrying %d more time(s)...", m.Retries-retry)
m.Log.Infof("Device busy! Retrying %d more time(s) on controller %q...", m.Retries-retry, m.Controller)
time.Sleep(time.Duration(m.RetriesWaitTime))
}
return m.gatherFields(requests)
Expand Down
4 changes: 2 additions & 2 deletions plugins/inputs/modbus/modbus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func TestRetryFailExhausted(t *testing.T) {

require.NoError(t, modbus.Gather(&acc))
require.Len(t, acc.Errors, 1)
require.ErrorContains(t, acc.FirstError(), "slave 1: modbus: exception '6' (server device busy)")
require.ErrorContains(t, acc.FirstError(), `slave 1 on controller "tcp://localhost:1502": modbus: exception '6' (server device busy)`)
}

func TestRetryFailIllegal(t *testing.T) {
Expand Down Expand Up @@ -287,7 +287,7 @@ func TestRetryFailIllegal(t *testing.T) {

require.NoError(t, modbus.Gather(&acc))
require.Len(t, acc.Errors, 1)
require.ErrorContains(t, acc.FirstError(), "slave 1: modbus: exception '1' (illegal function)")
require.ErrorContains(t, acc.FirstError(), `slave 1 on controller "tcp://localhost:1502": modbus: exception '1' (illegal function)`)
require.Equal(t, 1, counter)
}

Expand Down
2 changes: 1 addition & 1 deletion plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/init.err
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
configuration invalid: field "Voltage" duplicated in measurement "V"
field "Voltage" duplicated in measurement "V"
Loading