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

Alarm callback search results payload broken #923

Closed
bernd opened this Issue Jan 26, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@bernd
Member

bernd commented Jan 26, 2015

The search hits in an alarm callback are kinda broken. (See search_hits in payload below)

The serialized Message objects lack the id and other fields. There should be a separate DTO with Jackson annotations for this because this is an API. Also, the Message object needs to be built correctly including all fields.

Currently all fields of the raw object are exposed.

{
  "stream": {
    "id": "54c673fd87307f36d2441ef9",
    "title": "Radio messages",
    "alert_conditions": [
      {
        "id": "d763f409-c2b0-48e5-a77b-015c6d91c365",
        "created_at": "2015-01-26T17:11:21.741Z",
        "parameters": {
          "grace": 1,
          "time": 1,
          "backlog": 1,
          "threshold_type": "more",
          "threshold": 10
        },
        "creator_user_id": "bernd",
        "type": "message_count"
      }
    ],
    "description": "All messages from radios",
    "content_pack": null,
    "created_at": "2015-01-26T17:06:05.122Z",
    "outputs": [],
    "creator_user_id": "bernd",
    "rules": [
      {
        "id": "54c6740c87307f36d2441f0a",
        "field": "source",
        "value": "radio\\d?\\.example\\.org",
        "stream_id": "54c673fd87307f36d2441ef9",
        "type": 2,
        "inverted": false
      }
    ],
    "disabled": false
  },
  "check_result": {
    "result_description": "Stream had 4725 messages in the last 1 minutes with trigger condition more than 10 messages. (Current grace time: 1 minutes)",
    "triggered_condition": {
      "id": "d763f409-c2b0-48e5-a77b-015c6d91c365",
      "type": "MESSAGE_COUNT",
      "created_at": "2015-01-26T17:11:21.741Z",
      "creator_user_id": "bernd",
      "grace": 1,
      "parameters": {
        "grace": 1,
        "time": 1,
        "backlog": 1,
        "threshold_type": "more",
        "threshold": 10
      },
      "search_hits": [
        {
          "fields": {
            "http_response_code": 200,
            "gl2_source_node": "64c7a55d-2915-42bc-bbec-dfa416f645f2",
            "resource": "/posts/45326/edit",
            "http_method": "GET",
            "timestamp": "2015-01-26T17:35:02.874Z",
            "message": "GET /posts/45326/edit [200] 112ms",
            "took_ms": 112,
            "source": "radio.example.org",
            "gl2_source_input": "54c6352887307f36d243d92d",
            "gl2_source_radio_input": "54c636e287307f36d243db10",
            "action": "edit",
            "streams": [
              "54c673fd87307f36d2441ef9",
              "54c639f687307f36d243de60"
            ],
            "user_id": 6469981,
            "controller": "PostsController",
            "gl2_source_radio": "8711eb36-50ca-4a79-b335-5b05989accc5"
          },
          "streams": [],
          "source_input_id": null,
          "filter_out": false,
          "journal_offset": -9223372036854775808,
          "message": "GET /posts/45326/edit [200] 112ms",
          "id": null,
          "source": "radio.example.org",
          "field_names": [
            "http_response_code",
            "gl2_source_node",
            "resource",
            "http_method",
            "timestamp",
            "message",
            "took_ms",
            "source",
            "gl2_source_input",
            "gl2_source_radio_input",
            "action",
            "streams",
            "user_id",
            "controller",
            "gl2_source_radio"
          ],
          "complete": false,
          "validation_errors": "_id is missing, ",
          "fields_entries": [
            {"http_response_code": 200},
            {"gl2_source_node": "64c7a55d-2915-42bc-bbec-dfa416f645f2"},
            {"resource": "/posts/45326/edit"},
            {
              "http_method": "GET"
            },
            {
              "timestamp": "2015-01-26T17:35:02.874Z"
            },
            {
              "message": "GET /posts/45326/edit [200] 112ms"
            },
            {
              "took_ms": 112
            },
            {
              "source": "radio.example.org"
            },
            {
              "gl2_source_input": "54c6352887307f36d243d92d"
            },
            {
              "gl2_source_radio_input": "54c636e287307f36d243db10"
            },
            {
              "action": "edit"
            },
            {
              "streams": [
                "54c673fd87307f36d2441ef9",
                "54c639f687307f36d243de60"
              ]
            },
            {
              "user_id": 6469981
            },
            {
              "controller": "PostsController"
            },
            {
              "gl2_source_radio": "8711eb36-50ca-4a79-b335-5b05989accc5"
            }
          ],
          "field_count": 15,
          "stream_ids": [
            "54c673fd87307f36d2441ef9",
            "54c639f687307f36d243de60"
          ],
          "is_source_inet_address": false,
          "inet_address": null
        }
      ],
      "description": "time: 1, threshold_type: more, threshold: 10, grace: 1",
      "type_string": "MESSAGE_COUNT",
      "backlog": 1
    },
    "triggered_at": "2015-01-26T17:35:03.918Z",
    "triggered": true
  }
}

@bernd bernd added the bug label Jan 26, 2015

@bernd bernd added this to the 1.0.0 milestone Jan 26, 2015

@kroepke

This comment has been minimized.

Member

kroepke commented Jan 27, 2015

The underlying problem is that eventually this gets called:

public void addField(final String key, final Object value) {
    // Don't accept protected keys. (some are allowed though lol)
    if (RESERVED_FIELDS.contains(key) && !RESERVED_SETTABLE_FIELDS.contains(key) || !validKey(key)) {
 // ...
}

so that the id is not being set into the message.

@kroepke

This comment has been minimized.

Member

kroepke commented Jan 27, 2015

for HTTPAlarmCallback, the new structure of the messages that match the stream alert condition is the following:

    "matching_messages": [
      {
        "index": "graylog2_2",
        "message": "GET \/posts [200] 73ms",
        "fields": {
          "http_method": "GET",
          "took_ms": 73,
          "http_response_code": 200,
          "gl2_source_input": "54c77720d4c6b8c648cd1103",
          "gl2_source_node": "38048b35-034f-4212-9b1c-f7cec9f6acef",
          "action": "index",
          "resource": "\/posts",
          "user_id": 6469981,
          "controller": "PostsController"
        },
        "id": "b9eb78f0-a632-11e4-8869-8a3dffaa2055",
        "source": "example.org",
        "stream_ids": [
          "54c77687d4c6b8c648cd105f"
        ],
        "timestamp": "2015-01-27T14:42:32.575Z"
      }
    ]

the data is also accessible to other plugins, via AlertCondition.CheckResult#getMatchingMessages. The original method getSearchHits() on the AlertCondition is still present but deprecated and will be remove from a future release.

kroepke added a commit that referenced this issue Jan 27, 2015

force that every Message object has an id, even when created from Res…
…ultMessage

this happened in AlarmCallbacks, where Message objects were created without required fields (thanks to addFields which checked for _id…)

#923
@kroepke

This comment has been minimized.

Member

kroepke commented Jan 27, 2015

@lennartkoopmann do you want to add the new json format to the documentation?

@kroepke kroepke closed this in a93927b Jan 27, 2015

@dfch

This comment has been minimized.

dfch commented Feb 19, 2015

When I tried to adapt an AlarmCallback Plugin to the new structure, I found this issue and realized that the original Message object is no longer accessible. Though I can access the fields via getFields() I cannot use this for example to check for a field and retrieve a single field. In my opinion, this puts unnecessary burden on the consumer of this API (as the original Message object is available inside the MessageSummary (but private).

The method getMessage() only returns the message property of the Message object, but it would be much more practical to just return the (nearly) full message object (as it has been the case with getSearchHits(). Why can't there be a method to return the full object. This would make it much easier to work with. Ronald

@kroepke

This comment has been minimized.

Member

kroepke commented Feb 19, 2015

We encapsulated it because the Message class was in no way meant to be an API (and actually is a mess that will need serious refactoring).
I guess we can add a method in 1.0.1 that exposes the raw message, but will most likely immediately deprecate it.
However, if you are blocked by this, my suggestion would be to use reflection to make the property accessible.

@kroepke

This comment has been minimized.

Member

kroepke commented Mar 3, 2015

@dfch In which case are you unable to retrieve a field from MessageSummary?
We are debating wether to include #976 in 1.0.1 and would rather not to because it makes little sense to create a method and deprecate it immediately.
From what we can see all relevant fields should be available through MessageSummary. We do not want to make Message available right now because there's just too much cruft in there to be a proper API right now.

@dfch

This comment has been minimized.

dfch commented Mar 3, 2015

@kroepke maybe I was not clear on that, I can use getFields() to retrieve all fields. It is just that the original Message object supports methods like getField(String key) and hasField(String field). I could always get all fields and go through them my self, but to keep the semantics the same or similar I would suggest to just add these methods. So before you add something just to deprecate it immediately, I would favour to

  • either leave things as it is (and I can still use the other deprecated getSearchHits() method
  • or preferably add the two aforementioned methods getField() and hasFields() to MessageSummary

Side note: I do not understand why you do not want to make Message available to AlarmCallback plugins. Other plugins do have access to that as well (e.g. Filter, Output).

@kroepke

This comment has been minimized.

Member

kroepke commented Mar 3, 2015

The reason we moved it out of AlarmCallbacks was that the Message object was blindly serialized as JSON with "funny" results. We needed to fix that immediately. All other plugin types suffer from the same problems regarding the Message class "API", but we didn't have an immediate problem there.

Specifically, Message has references to other internal objects that plugins should really have no business dealing with (like MessageInput).

We can certainly add those two methods if that makes life easier.

@dfch

This comment has been minimized.

dfch commented Mar 3, 2015

It is just a thought, I certainly realise that there are more important issues waiting. So if you were going to change the MessageSummary later on anyway, I suggest you just leave it as it is (including to be able to use getSearchHits()) and once you complete the redesign, you change the whole thing once and forever. That way we would only have to update the code once. up to you ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment