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.

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

Get log entries with pagination #243

Merged
merged 4 commits into from
Apr 4, 2023

Conversation

xflipped
Copy link
Contributor

I've faced with the following situation when retrieving logs from LogService:

Link to entries:

  "@odata.type": "#LogService.v1_1_3.LogService",
  "Entries": {
    "@odata.id": "/redfish/v1/Managers/{ManagerID}/LogServices/{LogServiceID}/Entries"
  }

Actual entry collection object:

  "[email protected]": 52,
  "[email protected]": "/redfish/v1/Managers/{ManagerID}/LogServices/{LogServiceID}/Entries?$skip=50",
  "Name": "Log Service Entries Collection"

As you can see, this is the case when the original link leads only to the first N (here: N=50) entries. So I would like to propose changes that allow all entries to be retrieved.

I've also noticed that even though the log entry collection has a json field "Members" that normally contains links, here it contains a list of log entries directly:

Normally:

  "Members": [
    {
      "@odata.id": "/redfish/v1/Managers/{ManagerID}"
    }
  ],

Logs:

"Members": [
{
"@odata.id":view details "/redfish/v1/Managers/{ManagerID}/LogServices/{LogServiceID}/Entries/1",
"@odata.type":view details "#LogEntry.v1_14_0.LogEntry",
"Id":"1"
},
] 

So I think there is no need to query each log entry as a link. These changes can also be seen in my PR

@stmcginnis
Copy link
Owner

I've also noticed that even though the log entry collection has a json field "Members" that normally contains links, here it contains a list of log entries directly

This vendor is not following the Redfish specification:

"Entries": {
    "$ref": "http://redfish.dmtf.org/schemas/v1/LogEntryCollection.json#/definitions/LogEntryCollection",
    "description": "The link to the log entry collection.",
    "longDescription": "This property shall contain a link to a resource collection of type LogEntryCollection.",
    "readonly": true
},

An issue should be opened with that vendor asking that they fix their implementation to follow the spec.

@stmcginnis
Copy link
Owner

And the LogEntryCollection.Members defintion:

"Members": {
    "autoExpand": true,
    "description": "The members of this collection.",
    "items": {
        "$ref": "http://redfish.dmtf.org/schemas/v1/LogEntry.json#/definitions/LogEntry"
    },
    "longDescription": "This property shall contain an array of links to the members of this collection.",
    "readonly": true,
    "type": "array"
},

Copy link
Owner

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks close, and good overall. A few small things, and we need to make sure it is following the actual spec and not one vendors flawed implementation.

Thanks for working on this!

// MembersNextLink is the link used for pagination
MembersNextLink string `json:"[email protected]"`
// rawData holds the original serialized JSON so we can compare updates.
rawData []byte
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawData would not be needed here. That's only necessary when a structure has updatable fields where an Update() call is required.

}

// UnmarshalJSON unmarshals a LogEntry object from the raw JSON.
func (logEntryCollection *LogEntryCollection) UnmarshalJSON(b []byte) (err error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since rawData isn't needed to be captured, this custom marshalling can be removed completely.

@xflipped
Copy link
Contributor Author

xflipped commented Mar 31, 2023

Thanks for your comments!!

As for this
An issue should be opened with that vendor asking that they fix their implementation to follow the spec.

Please let me check this issue because I took the example directly from the dmtf website here:
https://redfish.dmtf.org/redfish/mockups/v1/1266#Systems--437XR1138R2--LogServices--Log1--Entries

@xflipped
Copy link
Contributor Author

xflipped commented Apr 4, 2023

I've added filtering as an option and vendor agnostic collection retrieval using links to follow the spec

@xflipped xflipped requested a review from stmcginnis April 4, 2023 12:43
Copy link
Owner

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xflipped!

@stmcginnis stmcginnis merged commit 628c786 into stmcginnis:main Apr 4, 2023
feuerrot pushed a commit to babiel/gofish that referenced this pull request May 24, 2023
* fix: get log entries

* fix: remove rawData

* feat: add filtering

* fix: collect entities
@GlqEason
Copy link

so it still need to query each log entry as a link ? i have tested some manufacturer device, and /redfish/v1/Managers/{ManagerId}/LogServices/{LogServiceId}/Entries api can got all enties completely

@xflipped
Copy link
Contributor Author

there's no need to query each log entry as a link, a collection will be retrieved correctly taking into account pagination
and as an option you can apply filtering (top, skip)

@GlqEason
Copy link

there's no need to query each log entry as a link, a collection will be retrieved correctly taking into account pagination and as an option you can apply filtering (top, skip)

sorry, i still don't quite understand, and which function can i use to get the latest 10 log entries,at least i should know how many logs it has ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants