Solr plugin #2019

ljagiello · 2016-11-09T07:48:30Z

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

Solr plugin collecting metrics from Apache Solr server.

Issue (#278)

I mess up with a previous PR(#1734)

phemmer · 2017-01-10T17:45:30Z

plugins/inputs/solr/solr.go

+			continue
+		}
+		coreFields := map[string]interface{}{
+			"class_name":   metrics.Class,


class_name should probably be a tag, not a normal field.

@phemmer I feel like those class names are a bit useless in tags but just in case I left them as fields. If there is any good reason to keep them as tags, yeah why not I can convert them to tags.

Let's either go with tag, or just exclude them for now if they are not useful.

phemmer · 2017-01-10T17:53:44Z

plugins/inputs/solr/solr.go

+			wg.Add(1)
+			go func(serv string, core string, acc telegraf.Accumulator) {
+				defer wg.Done()
+				if err := s.gatherCoreStats(fmt.Sprintf("%s/solr/%s"+mbeansPath, serv, core), core, acc); err != nil {


Why the mixture of string formatting, and string concatenation? Why not "%s/solr/%s%s", serv, core, mbeansPath)?

phemmer · 2017-01-10T17:56:20Z

plugins/inputs/solr/solr.go

+	Class string `json:"class"`
+	Stats struct {
+		CumulativeEvictions int     `json:"cumulative_evictions"`
+		CumulativeHitratio  float64 `json:"cumulative_hitratio,string"`


This is throwing:

Error in plugin [inputs.solr]: cache category: json: invalid use of ,string struct tag, trying to unmarshal unquoted value into float64

@phemmer could you please provide output of:

http://[server]:8983/solr/admin/mbeans?stats=true&wt=json&cat=CORE&cat=QUERYHANDLER&cat=UPDATEHANDLER&cat=CACHE

it works fine for me and I don't have such error. Against what version of Solr server are you testing it?

I have SOLR 5.3.1 and could reproduce the issue. I found the values at:

http://[SERVER]:8983/solr/[CORE]/admin/mbeans?stats=true&wt=json&cat=CORE&cat=QUERYHANDLER&cat=UPDATEHANDLER&cat=CACHE

An extract of the whole JSON is:

{ "solr-mbeans": [ { "filterCache": { "class": "org.apache.solr.search.FastLRUCache", "version": "1.0", "description": "Concurrent LRU Cache(maxSize=512, initialSize=512, minSize=460, acceptableSize=486, cleanupThread=false)", "src": null, "stats": { "lookups": 6, "hits": 0, "hitratio": 0, "inserts": 6, "evictions": 0, "size": 6, "warmupTime": 0, "cumulative_lookups": 6017884, "cumulative_hits": 9729, "cumulative_hitratio": 0, "cumulative_inserts": 6008155, "cumulative_evictions": 3110 } } } ] }

so I have the same issue with the hitratio field. When I remove ',string' for both it seems to work.

phemmer · 2017-01-10T18:00:43Z

plugins/inputs/solr/solr.go

+			}
+		default:
+			err := errors.New("unrecognized category")
+			acc.AddError(fmt.Errorf("category: %s", err))


These 2 lines should be merged or at least handled differently. Line 397 creates err, but then 398 converts err into a string, losing any reason for creating an error object.

phemmer · 2017-01-10T18:02:58Z

plugins/inputs/solr/solr.go

+func gatherCacheMetrics(mbeansJSON json.RawMessage, core, category string, acc telegraf.Accumulator) error {
+	var coreMetrics map[string]Cache
+
+	measurementTime := time.Now()


All the metrics were retrieved at the exact same time. Instead of creating a separate timestamp for each measurement, I'd use the same timestamp for all. It'll make it a lot easier for users to correlate the metrics without having to deal with time windows/tolerances.

phemmer · 2017-01-10T18:14:44Z

plugins/inputs/solr/solr.go

+			acc.AddError(fmt.Errorf("JSON fetch error: %s", err))
+		}
+		switch category {
+		case "CORE":


This feels rather odd, creating a static slice, iterating it, and then switching on which one you're currently iterating on. What about a function you pass the appropriate gather func to. For example:

err = s.gatherCategory(url, core, "CORE", gatherCoreMetrics, acc) err = s.gatherCategory(url, core, "QUERYHANDLER", gatherQueryHandlerMetrics, acc) ...

phemmer · 2017-01-10T18:27:28Z

plugins/inputs/solr/solr.go

+}
+
+// Default settings
+func (s *Solr) setDefaults() ([][]string, int, error) {


Perhaps a better name for this function? It doesn't seem to be dealing much with defaults, just getting a list of cores to enumerate.

phemmer · 2017-01-10T18:35:21Z

plugins/inputs/solr/solr.go

+		}
+	}
+	wg.Wait()
+	return errChan.Error()


This is going to result in only the first error being shown to the user (all others discarded). acc.AddError can be used here which will solve the issue. returning nil is fine if errors were added through acc.AddError.

phemmer · 2017-01-10T18:48:25Z

Oh, several fields on the replication handler are not gathered:

          "indexSize": "71 bytes",
          "indexVersion": 0,
          "generation": 1,
          "indexPath": "/var/lib/solr/cores/MC_1_CatalogEntry/data/index/",
          "isMaster": "true",
          "isSlave": "false",
          "replicateAfter": [
            "commit"
          ],
          "replicationEnabled": "true"

Instead of hard-coding a list of struct fields to unmarshal, perhaps the plugin should just gather the stats field, and just unmarshal it into a map[string]interface{}, and the pass that into acc.AddFields().
The only catch is that it might result in fields getting created in influxdb as int (such as with 0 values), and later telegraf tries inserting a float (when it's non-0). However we can probably solve this by just looping through the stats and if the field name ends in Time or PerSecond, explicitly convert it to a float.
Doing this would significantly reduce the code size of the plugin, and make it future proof in case more fields are ever added.

danielnelson

@ljagiello This one is close to ready, if we can resolve the remaining comments then we should be able to merge this plugin.

danielnelson · 2017-10-26T21:10:54Z

plugins/inputs/solr/README.md

+* Plugin: solr, Collection 1
+> solr_core,core=main,handler=searcher,host=testhost class_name="org.apache.solr.search.SolrIndexSearcher",deleted_docs=17616645i,max_docs=261848363i,num_docs=244231718i 1478214949000000000
+> solr_core,core=main,handler=core,host=testhost class_name="main",deleted_docs=0i,max_docs=0i,num_docs=0i 1478214949000000000
+> solr_queryhandler,core=main,handler=/replication,host=testhost 15min_rate_reqs_per_second=0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000444659081257,5min_rate_reqs_per_second=0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000014821969375,75th_pc_request_time=16.484211,95th_pc_request_time=16.484211,999th_pc_request_time=16.484211,99th_pc_request_time=16.484211,avg_requests_per_second=0.0000008443809966322143,avg_time_per_request=12.984811,class_name="org.apache.solr.handler.ReplicationHandler",errors=0i,handler_start=1474662050865i,median_request_time=11.352427,requests=3i,timeouts=0i,total_time=38.954433 1478214949000000000


This is not a problem with this pull request, but that float format is silly. Seems like we should switch to using the 'g' fmt with FormatFloat.

danielnelson · 2017-10-26T21:14:44Z

plugins/inputs/solr/solr.go

+	Status map[string]struct {
+		Index struct {
+			SizeInBytes int64 `json:"sizeInBytes"`
+			NumDocs     int   `json:"numDocs"`


I would use int64 thoughout, all int fields are store das int64 internally and this can prevent differences with 32-bit systems. It is not required though if the value cannot be larger than 32-bits.

danielnelson · 2017-10-26T21:47:25Z

plugins/inputs/solr/solr.go

+			continue
+		}
+		coreFields := map[string]interface{}{
+			"class_name":   metrics.Class,


Let's either go with tag, or just exclude them for now if they are not useful.

ljagiello · 2017-10-26T21:59:39Z

@danielnelson I will check that over the weekend.

ljagiello · 2017-10-30T04:48:37Z

@danielnelson all your comments are fixed.

There was a valid comment earlier (#2019 (comment)) about 2 different results in hitratio (different between different version of solr) - it can be string eg.: "0.79" or int eg.: 0. I can add an extra interface and map based on type or is there a better approach for that?

danielnelson · 2017-10-30T19:47:57Z

I think this is what you will need to do, I don't know of any alternative.

Solr plugin collecting metrics from Apache Solr server.

- removed `class_name` - `int` replaced with `int64` - golint fixes

ljagiello · 2017-11-06T18:51:42Z

@danielnelson ok should be fine now.

ljagiello · 2017-11-07T19:20:30Z

Tested with Solr 4.3, 5.5.5, 6.6 (Solr 7.x use a different schema now).

danielnelson · 2017-11-07T23:22:21Z

plugins/inputs/solr/solr.go

@@ -345,22 +349,49 @@ func addUpdateHandlerMetricsToAcc(acc telegraf.Accumulator, core string, mBeansD
 	return nil
 }

+// Get float64 from interface
+func getFloat(unk interface{}) (float64, error) {


Remove error since it is never used.

I'm under the impression that since you are storing into an interface, the only possibilities are float64 or string, and other types that would indicate unexpected JSON (bool, nil, []interface{}, map[string]interface{}), so you could probably shorten this list.

danielnelson · 2017-11-08T00:50:44Z

Will be in 1.5, thanks!

ljagiello mentioned this pull request Nov 9, 2016

Solr plugin #1734

Closed

3 tasks

sparrc modified the milestones: 1.2.0, 1.3.0 Nov 9, 2016

sparrc added the plugin request label Nov 26, 2016

phemmer reviewed Jan 10, 2017

View reviewed changes

sparrc modified the milestones: Future Milestone, 1.3.0 Jan 24, 2017

danielnelson modified the milestones: 1.4.0, Future Milestone Apr 13, 2017

danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin request labels Aug 12, 2017

danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017

danielnelson added the new plugin label Sep 18, 2017

danielnelson reviewed Oct 26, 2017

View reviewed changes

Lukasz Jagiello added 3 commits November 5, 2017 16:48

Solr plugin

eaf7d60

Solr plugin collecting metrics from Apache Solr server.

PR cleanup

da97e80

- removed `class_name` - `int` replaced with `int64` - golint fixes

Fix for field type

6c0679e

danielnelson reviewed Nov 7, 2017

View reviewed changes

Shorten version of getFloat

cbace99

Removed error from getFloat

d14436e

danielnelson merged commit 493af04 into influxdata:master Nov 8, 2017

ljagiello deleted the solr-plugin branch November 8, 2017 00:54

mkboudreau mentioned this pull request Jan 16, 2018

Add Solr 3 plugin or compatibility #3681

Closed

aromeyer pushed a commit to aromeyer/telegraf that referenced this pull request May 19, 2018

Add Solr input plugin (influxdata#2019)

2296d92

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add Solr input plugin (#2019)

89650bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solr plugin #2019

Solr plugin #2019

ljagiello commented Nov 9, 2016 •

edited

Loading

phemmer Jan 10, 2017

ljagiello Jan 19, 2017

danielnelson Oct 26, 2017

phemmer Jan 10, 2017

phemmer Jan 10, 2017

ljagiello Jan 19, 2017

buschjost Mar 6, 2017 •

edited

Loading

phemmer Jan 10, 2017

phemmer Jan 10, 2017

phemmer Jan 10, 2017

phemmer Jan 10, 2017

phemmer Jan 10, 2017

phemmer commented Jan 10, 2017 •

edited

Loading

danielnelson left a comment

danielnelson Oct 26, 2017

danielnelson Oct 26, 2017

danielnelson Oct 26, 2017

ljagiello commented Oct 26, 2017

ljagiello commented Oct 30, 2017 •

edited

Loading

danielnelson commented Oct 30, 2017

ljagiello commented Nov 6, 2017

ljagiello commented Nov 7, 2017

danielnelson Nov 7, 2017

ljagiello Nov 7, 2017

danielnelson commented Nov 8, 2017

Solr plugin #2019

Solr plugin #2019

Conversation

ljagiello commented Nov 9, 2016 • edited Loading

Required for all PRs:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschjost Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phemmer commented Jan 10, 2017 • edited Loading

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljagiello commented Oct 26, 2017

ljagiello commented Oct 30, 2017 • edited Loading

danielnelson commented Oct 30, 2017

ljagiello commented Nov 6, 2017

ljagiello commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented Nov 8, 2017

ljagiello commented Nov 9, 2016 •

edited

Loading

buschjost Mar 6, 2017 •

edited

Loading

phemmer commented Jan 10, 2017 •

edited

Loading

ljagiello commented Oct 30, 2017 •

edited

Loading